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

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2023-11-07 at 08:04 +0100, Simon Sobisch wrote:
> Thank you very much for this proof-of-concept use!
> 
> Inspecting it raises the following questions to me, both for a
> possible 
> binutils implementation and for the library use in general:
> 
> * How should the application set the relevant context (often lines
> are
>    shown before/after)?

Currently the source-printing code has this logic (in gcc/diagnostic-
show-locus.cc):
- filter locations within the diagnostic to "sufficiently sane" ones
(thus ignoring e.g. ranges that end before they start)
- look at all of the remaining locations that are in the same source
file as the primary location of the diagnostic
- determine a set of runs of source lines; layout::calculate_line_spans
has this comment:

/* We want to print the pertinent source code at a diagnostic.  The
   rich_location can contain multiple locations.  This will have been
   filtered into m_exploc (the caret for the primary location) and
   m_layout_ranges, for those ranges within the same source file.

   We will print a subset of the lines within the source file in question,
   as a collection of "spans" of lines.

   This function populates m_line_spans with an ordered, disjoint list of
   the line spans of interest.

   Printing a gap between line spans takes one line, so, when printing
   line numbers, we allow a gap of up to one line between spans when
   merging, since it makes more sense to print the source line rather than a
   "gap-in-line-numbering" line.  When not printing line numbers, it's
   better to be more explicit about what's going on, so keeping them as
   separate spans is preferred.

   For example, if the primary range is on lines 8-10, with secondary ranges
   covering lines 5-6 and lines 13-15:

     004
     005                   |RANGE 1
     006                   |RANGE 1
     007
     008  |PRIMARY RANGE
     009  |PRIMARY CARET
     010  |PRIMARY RANGE
     011
     012
     013                                |RANGE 2
     014                                |RANGE 2
     015                                |RANGE 2
     016

   With line numbering on, we want two spans: lines 5-10 and lines 13-15.

   With line numbering off (with span headers), we want three spans: lines 5-6,
   lines 8-10, and lines 13-15.  */
(end of quote)

This algorithm could be tweaked if you want extra lines of context,
perhaps having an integer option N for N extra lines of before/after
context around each run.


> * Should it be possible to override msgid used to display the
>    warning/error type?
>    If this would be possible then the text sink in messages_init may
> be
>    adjusted to replace the label with _("Warning") and _("Error"),
> which
>    would leave the text output "as-is" (if the text sink is
> configured to
>    not output the source line); this would make it usable without
>    adjusting the testsuite and to adopt to a standard later.

For the msgids, I was more thinking of the messages of the diagnostics
themselves; for instance, in GCC the error format string:

   "Invalid argument %d for builtin %qF"

has fr.po translation:

   "Argument %d invalide pour la fonction interne %qF"

But it sounds like gas also has capitalized severities (e.g.
"Warning"), so maybe that should simply be a flag in the text sink.

> 
> 
> Notes for the SARIF output:
> * the region contains an error, according to the linked JSON spec
>    startColumn has a minimum of 1 (I guess you'd just leave it away
> if
>    the application did not set it)

Good catch; looks like a bug in my SARIF output code.  I've filed it
for myself as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112425


> * the application should have the option to pre-set the
> sourceLanguage
>    for the diagnostic_manager (maybe even make that a positional
> argument
>    that needs to be passed but can be NULL) and override it when
>    specifying a region

Why?

Note that the sourceLanguage can always be NULL.  I considered setting
it for gas, but it's not clear what the value can be, so I just omit
it; see:
  https://github.com/oasis-tcs/sarif-spec/issues/608



[..snip...]

Thanks for the feedback
Dave