Re: [PATCH] binutils: experimental use of libdiagnostics in gas

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2023-11-07 at 10:21 +0100, Clément Chigot wrote:
> Hi David,
> 
> Thanks for that interesting RFC ! I'm fully in favor of such
> improvements, having uniformed error messages across gcc, gas and
> later ld, would greatly help integration of these tools, let alone
> the
> SARIF format output.

Indeed, I can imagine that ld might eventually want to use this as
well.

> However, I'm not sure how you're planning to make the transition. But
> currently, it looks like libdiagnostics is either enabled and thus
> the
> new format being produced, either it's not and we do have the legacy
> format. I think the transition should be smoother than that, there
> are
> probably thousands of tests, scripts, whatever out in the wild
> expecting this legacy format. Allowing both formats within the same
> executable, basically chosen by a flag, would probably ease the
> transition.

Yes.  I'm assuming that consumers of libdiagnostics would have a
configure-time test for the availability of libdiagnostics.  In the
example I gave, it was just a compile-time "choice" (I'm not an expert
at autotools, so I hacked all of that up for now)... but if the feature
is available, it could be a run-time choice.

We've been adding new features to GCC's diagnostic output over the
years (adding column numbers, showing macro expansions, quoting source
code with underlines, fix-it hints, etc), but each time we've added a
flag to turn them off (e.g. -fno-diagnostics-show-line-numbers,  -fno-
diagnostics-show-labels, etc).

As of GCC 11 we have a -fdiagnostics-plain-output which "requests that
diagnostic output look as plain as possible, which may be useful when
running dejagnu or other utilities that need to parse diagnostics
output and prefer that it remain more stable over time."

In the implementation patch I made the text sink turn on everything by
default here:
  m_dc.m_source_printing.enabled = true; // FIXME
  m_dc.m_source_printing.colorize_source_p = true; // FIXME
  m_dc.m_source_printing.show_labels_p = true; // FIXME
  m_dc.m_source_printing.show_line_numbers_p = true; // FIXME
  m_dc.m_source_printing.min_margin_width = 6; // FIXME
and I didn't provide a way of turning things off.  So maybe the API
needs a way to tweak options of the text sink?  Maybe:

  diagnostic_text_sink_set_source_printing (sink, true);
  diagnostic_text_sink_set_colorize_source (sink, COLORIZE_IF_AT_TTY);

...etc.

Also, I made no particular effort to make the output similar to before,
hence e.g. the difference in capitalization "Error: " vs "error: ".  Is
that capitalization something that you'd want to remain consistent?

> 
> Apart from that, just a few remarks on the implementation details,
> see below.

[...snip...]

> > @@ -101,6 +109,29 @@ had_warnings (void)
> >    return warning_count;
> >  }
> > 
> > +#if USE_LIBDIAGNOSTICS
> > +static diagnostic_manager *diag_mgr;
> 
> Would it make sense for an application to have several
> "diagnostic_manager" ? 
> If no, I'm wondering if this variable shouldn't
> be hidden inside libdiagnostics itself, avoiding every calls to have
> this diag_mgr argument.

Although it might not make sense for binutils-style use-cases to have
multiple diagnostic_manager instances (since these are implemented all
standalone programs), I think in general it *does* make sense.

I've found it's usually a bad idea for a shared library to have global
state, since at some point a consumer of the library is a shared
library itself, at which point users of the 2nd library see unexpected
interactions.

Consider the case of a linting tool implemented as a shared library:
the tool has no knowledge of where it's going to be embedded: e.g. in
an IDE, or as part of some other system.  Perhaps the IDE is
multithreaded.  So I think it's better for the user of the diagnostic
API (here the lint tool) to have an explicit "context" pointer that
it's sending diagnostics to, rather than having it be implicit inside
the library.


> 
> > +#endif
> > +
> > +void messages_init (void)
> > +{
> > +#if USE_LIBDIAGNOSTICS
> > +  diag_mgr = diagnostic_manager_new ();
> > +  diagnostic_manager_add_text_sink (diag_mgr, stderr,
> > +                                   DIAGNOSTIC_COLORIZE_IF_TTY);
> > +  diagnostic_manager_add_sarif_sink (diag_mgr, stderr,
> > +                                   
> > DIAGNOSTIC_SARIF_VERSION_2_1_0);
> > +#endif
> > +}
> > +
> > +void messages_end (void)
> > +{
> > +#if USE_LIBDIAGNOSTICS
> > +  diagnostic_manager_release (diag_mgr);
> 
> IIUC, diagnostic_manager_release must be called to produce any
> output.

The text sink emits (and flushes) each diagnostic to the FILE * stream
after diagnostic_finish is called on it.

The sarif sink accumulates a JSON representation in memory, and only
writes to its FILE * after the manager is released (since there are
aspects of the metadata part of the format that requiring knowing about
all diagnostics upfront).

> However, nothing prevents the application to exit earlier see
> "as_fatal". Thus, this probably need to be called using atexit to
> ensure that whatever happens the messages are being outputted.

atexit handlers are per-process global state, so I'm thinking that
being something the client would register, rather than libdiagnostics
doing it automatically.


[...snip...]

Thanks for the feedback; hope the above sounds reasonable
Dave