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

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2023-11-07 at 11:03 +0100, Jan Beulich wrote:
> On 06.11.2023 23:29, David Malcolm wrote:
> > Here's a patch for gas in binutils that makes it use libdiagnostics
> > (with some nasty hardcoded paths to specific places on my hard
> > drive
> > to make it easier to develop the API).
> > 
> > For now this hardcodes adding two sinks: a text sink on stderr, and
> > also a SARIF output to stderr (which happens after all regular
> > output).
> > 
> > For example, without this patch:
> > 
> >    gas testsuite/gas/all/warn-1.s
> > 
> > emits:
> > VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> > VVVVVVVVV
> > testsuite/gas/all/warn-1.s: Assembler messages:
> > testsuite/gas/all/warn-1.s:3: Warning: a warning message
> > testsuite/gas/all/warn-1.s:4: Error: .warning argument must be a
> > string
> > testsuite/gas/all/warn-1.s:5: Warning: .warning directive invoked
> > in source file
> > testsuite/gas/all/warn-1.s:6: Warning: .warning directive invoked
> > in source file
> > testsuite/gas/all/warn-1.s:7: Warning:
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > ^^^^^^^^^
> > 
> > whereas with this patch:
> >   LD_LIBRARY_PATH=/home/david/coding-3/gcc-newgit-canvas-
> > 2023/build/gcc ./as-new testsuite/gas/all/warn-1.s
> > emits:
> > 
> > VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> > VVVVVVVVV
> > testsuite/gas/all/warn-1.s:3: warning: a warning message
> >     3 |  .warning "a warning message"   ;# { dg-warning "Warning: a
> > warning message" }
> >       |
> > testsuite/gas/all/warn-1.s:4: error: .warning argument must be a
> > string
> >     4 |  .warning a warning message     ;# { dg-error "Error:
> > .warning argument must be a string" }
> >       |
> > testsuite/gas/all/warn-1.s:5: warning: .warning directive invoked
> > in source file
> >     5 |  .warning                       ;# { dg-warning "Warning:
> > .warning directive invoked in source file" }
> >       |
> > testsuite/gas/all/warn-1.s:6: warning: .warning directive invoked
> > in source file
> >     6 |  .warning ".warning directive invoked in source file"   ;#
> > { dg-warning "Warning: .warning directive invoked in source file" }
> >       |
> > testsuite/gas/all/warn-1.s:7: warning:
> >     7 |  .warning ""                    ;# { dg-warning "Warning: "
> > }
> >       |

[...snip...]

> > which I see:
> > - drops the leading "Assembler messages" warning,
> > - changes the capitalization of the "Warning" -> "warning" etc
> > - quotes the pertinent line in the .s file
> > 
> > All of the locations are just lines; does gas do column numbers at
> > all?
> > (or ranges?)
> 
> It currently doesn't, which is primarily related to the scrubbing
> done
> before lines are actually processed.

How complicated/desirable would it be to track locations in .s files at
the column level?  I confess I didn't look at the parsing code at all.

> 
> I take it that the lack of column information is why there are lines
> of
> this form
> 
>       |
> 
> in the example output above. 

Yes: those lines are for annotation information such as underlining
specific columns.

> Them uniformly not carrying any information
> would make it desirable for them to be suppressed.

In GCC we typically have column information, so I'd never noticed this
behavior, but it ought to be fixable, to simply not display these if
there's no column info.

> 
> > @@ -172,16 +203,34 @@ as_tsktsk (const char *format, ...)
> >  static void
> >  as_warn_internal (const char *file, unsigned int line, char
> > *buffer)
> >  {
> > +#if !USE_LIBDIAGNOSTICS
> >    bool context = false;
> > +#endif
> >  
> >    ++warning_count;
> >  
> >    if (file == NULL)
> >      {
> >        file = as_where_top (&line);
> > +#if !USE_LIBDIAGNOSTICS
> >        context = true;
> > +#endif
> 
> I can't spot how this context information would be replaced. It works
> for macros only right now, but the hope is to eventually extend it
> also to .include files.

I confess I hacked this up, and I didn't check what this code does.
I see now that it calls as_report_context, which iterates over macro
expansions calling as_info_where with successively larger "indent"
values.

I could extend the patch to cover that.

More ambitiously, GCC's location tracking supports recording macro
expansions and include files, and the diagnostics subsystem has a way
of printing this information.  So potentially libdiagnostics could
provide API support for this - but I haven't yet looked at the
feasibility.

> 
> > @@ -199,6 +248,7 @@ as_warn_internal (const char *file, unsigned
> > int line, char *buffer)
> >  #ifndef NO_LISTING
> >    listing_warning (buffer);
> >  #endif
> > +#endif /* #else clause of #if USE_LIBDIAGNOSTICS */
> 
> This listing integration of course needs to remain irrespective of
> which way of emitting diagnostics is used.

Likewise; I think I just put the #endif in the wrong place above.

Thanks for the feedback; hope this is constructive.
Dave