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

["Clément Chigot" <chigot@adacore.com>]

> > 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."

I guess starting by a configure-time choice before such flags like
-fdiagnostics-plain-output are implemented is enough. I merely wish
that there is a way to preserve the old output, giving people the time
to experiment and then transitioning all their tools.

We can also introduce a flag like "-fdiagnostics-legacy-output`.
Though, I'm fearing it will never be removed, meaning maintaining the
previous code forever... A configure-time choice can be more easily
enabled by default in a few versions and then removed completely after
another bunch of versions.

> 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?

If we keep a way to produce the old output, I don't think so. And it
would probably be better to be coherent across gcc, gas, etc.

> >
> > 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.

Thanks for the explanation. I'm fine with that now.

> >
> > > +#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.

Arf, missed that sorry.

> 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.

Yes, we would probably need something like that here. Otherwise, we
would miss some sink outputs if they can be completed only at the end,
making them usable.

> [...snip...]
>
> Thanks for the feedback; hope the above sounds reasonable
> Dave

It does !

Thanks,
Clément