[7/9] Add RTL-error-handling to host

Link to previous discussion:
  https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00648.html

On Mon, 2016-10-10 at 19:53 +0100, Richard Sandiford wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
> > On Wed, 2016-10-05 at 18:00 +0200, Bernd Schmidt wrote:
> > > On 10/05/2016 06:15 PM, David Malcolm wrote:
> > > > 	* errors.c: Use consistent pattern for bconfig.h vs
> > > > config.h
> > > > 	includes.
> > > > 	(progname): Wrap with #ifdef GENERATOR_FILE.
> > > > 	(error): Likewise.  Add "error: " to message.
> > > > 	(fatal): Likewise.
> > > > 	(internal_error): Likewise.
> > > > 	(trim_filename): Likewise.
> > > > 	(fancy_abort): Likewise.
> > > > 	* errors.h (struct file_location): Move here from read
> > > > -md.h.
> > > > 	(file_location::file_location): Likewise.
> > > > 	(error_at): New decl.
> > >
> > > Can you split these out into a separate patch as well? I'll
> > > require
> > > more
> > > explanation for them and they seem largely independent.
> >
> > [CCing Richard Sandiford]
> >
> > The gen* tools have their own diagnostics system, in errors.c:
> >
> > /* warning, error, and fatal.  These definitions are suitable for
> > use
> >    in the generator programs; the compiler has a more elaborate
> > suite
> >    of diagnostic printers, found in diagnostic.c.  */
> >
> > with file locations tracked using read-md.h's struct file_location,
> > rather than location_t (aka libcpp's source_location).
> >
> > Implementing an RTL frontend by using the RTL reader from read
> > -rtl.c
> > means that we now need a diagnostics subsystem on the *host* for
> > handling errors in RTL files, rather than just on the build
> > machine.
> >
> > There seem to be two ways to do this:
> >
> >   (A) build the "light" diagnostics system (errors.c) for the host
> > as
> > well as build machine, and link it with the RTL reader there, so
> > there
> > are two parallel diagnostics subsystems.
> >
> >   (B) build the "real" diagnostics system (diagnostics*) for the
> > *build* machine as well as the host, and use it from the gen*
> > tools,
> > eliminating the "light" system, and porting the gen* tools to use
> > libcpp for location tracking.
> >
> > Approach (A) seems to be simpler, which is what this part of the
> > patch
> > does.
> >
> > I've experimented with approach (B).  I think it's doable, but it's
> > much more invasive (perhaps needing a libdiagnostics.a and a
> > build/libdiagnostics.a in gcc/Makefile.in), so I hope this can be
> > followup work.
> >
> > I can split the relevant parts out into a separate patch, but I was
> > wondering if either of you had a strong opinion on (A) vs (B)
> > before I
> > do so?
>
> (A) sounds fine to me FWIW.  And sorry for the slow reply.
>
> Thanks,
> Richard

This patch implements approach (A).

It updates the error messages to contain "error: " so that they can
be handled by DejaGnu.

It also adds an "error_at" function to the "light" API.  Doing so requires
moving the decl of struct file_location to errors.h.

gcc/ChangeLog:
	* Makefile.in (OBJS): Add errors.o.
	* errors.c: Use consistent pattern for bconfig.h vs config.h
	includes.
	(progname): Wrap with #ifdef GENERATOR_FILE.
	(error): Likewise.  Add "error: " to message.
	(fatal): Likewise.
	(internal_error): Likewise.
	(trim_filename): Likewise.
	(fancy_abort): Likewise.
	* errors.h (struct file_location): Move here from read-md.h.
	(file_location::file_location): Likewise.
	(error_at): New decl.
	* read-md.h (struct file_location): Move to errors.h.
	(file_location::file_location): Likewise.
	Include errors.h.
---
 gcc/Makefile.in |  1 +
 gcc/errors.c    | 23 +++++++++++++++++------
 gcc/errors.h    | 14 ++++++++++++++
 gcc/read-md.h   | 14 +-------------
 4 files changed, 33 insertions(+), 19 deletions(-)

David Malcolm <dmalcolm@redhat.com> writes:
> +inline file_location::file_location (const char *filename_in, int lineno_in, int colno_in)
> +: filename (filename_in), lineno (lineno_in), colno (colno_in) {}
> +

Long line (a pre-existing problem, since you're just moving the code).

I'm happy with this FWIW, but it'd be a stretch to say the whole thing
comes under the gen* umbrella.

Thanks,
Richard

Been looking at this off and on, and I'm still not sure I entirely get 
it - sorry.

On 11/11/2016 10:15 PM, David Malcolm wrote:
>>> Implementing an RTL frontend by using the RTL reader from read
>>> -rtl.c
>>> means that we now need a diagnostics subsystem on the *host* for
>>> handling errors in RTL files, rather than just on the build
>>> machine.

So, there are two things that bother me about this patch description:
  - The host already has the full diagnostic subsystem. The fact that
    you're commenting out some of the functions in errors.c suggests
    that errors.c is conflicting with the full one.
  - We already compile errors.c for both build and host.

Is there a problem with using both the full and the light errors system 
for read-rtl, as available? Mismatches in function signatures or 
something like this?

> -#ifdef HOST_GENERATOR_FILE
> -#include "config.h"
> -#define GENERATOR_FILE 1
> -#else
> +/* This file is compiled twice: once for the generator programs
> +   once for the compiler.  */
> +#ifdef GENERATOR_FILE
>  #include "bconfig.h"
> +#else
> +#include "config.h"
>  #endif
>  #include "system.h"
>  #include "errors.h"

The Makefile still has a HOST_GENERATOR_FILE definition for errors.c 
after this.


Bernd

On Mon, 2016-11-28 at 14:47 +0100, Bernd Schmidt wrote:
> Been looking at this off and on, and I'm still not sure I entirely
> get 
> it - sorry.
> 
> On 11/11/2016 10:15 PM, David Malcolm wrote:
> > > > Implementing an RTL frontend by using the RTL reader from read
> > > > -rtl.c
> > > > means that we now need a diagnostics subsystem on the *host*
> > > > for
> > > > handling errors in RTL files, rather than just on the build
> > > > machine.
> 
> So, there are two things that bother me about this patch description:
>   - The host already has the full diagnostic subsystem.

Maybe I worded this poorly.

I meant to say:

"we now need
  ((a diagnostics subsystem for handling errors in RTL files)
   on the *host*),
rather than just on the build machine."

rather than:

"we now need a
  ((diagnostics subsystem on the *host*)
   for handling
errors in RTL files),
 rather than just on the build machine."

if that distinction makes sense.  Clearly we already have a diagnostics
subsystem on the host; what this patch is adding is the separate, rtl-s
pecific diagnostic subsystem to cc1 on the host.

> The fact that
>     you're commenting out some of the functions in errors.c suggests
>     that errors.c is conflicting with the full one.

It doesn't conflict: C++ overloading allows both to co-exist.  However
I wanted to make sure that we don't accidentally use the RTL-specific
error-handling within other parts of the compiler.

>   - We already compile errors.c for both build and host.

Aha, yes, we do, it's linked into gengtype on the host to allow plugins
to support GTY.  The patch adds it to OBJS so that it is available
within cc1.

> Is there a problem with using both the full and the light errors
> system 
> for read-rtl, as available? Mismatches in function signatures or 
> something like this?

As noted above, C++ overloading allows this.

> > -#ifdef HOST_GENERATOR_FILE
> > -#include "config.h"
> > -#define GENERATOR_FILE 1
> > -#else
> > +/* This file is compiled twice: once for the generator programs
> > +   once for the compiler.  */
> > +#ifdef GENERATOR_FILE
> >  #include "bconfig.h"
> > +#else
> > +#include "config.h"
> >  #endif
> >  #include "system.h"
> >  #include "errors.h"
> 
> The Makefile still has a HOST_GENERATOR_FILE definition for errors.c 
> after this.

Will remove.

On 11/29/2016 06:20 PM, David Malcolm wrote:
>
> if that distinction makes sense.  Clearly we already have a diagnostics
> subsystem on the host; what this patch is adding is the separate, rtl-s
> pecific diagnostic subsystem to cc1 on the host.

So that still seems odd to me. Why not use the normal diagnostics 
subsystem, and add whatever you need from it to errors.c for use from 
the generator programs? What exactly makes it "rtl-specific"?


Bernd

On Tue, 2016-11-29 at 18:23 +0100, Bernd Schmidt wrote:
> On 11/29/2016 06:20 PM, David Malcolm wrote:
> > 
> > if that distinction makes sense.  Clearly we already have a
> > diagnostics
> > subsystem on the host; what this patch is adding is the separate,
> > rtl-s
> > pecific diagnostic subsystem to cc1 on the host.
> 
> So that still seems odd to me. Why not use the normal diagnostics 
> subsystem, and add whatever you need from it to errors.c for use from
> the generator programs? What exactly makes it "rtl-specific"?

The main issue is that the normal diagnostics subsystem tracks
locations using location_t (aka libcpp's source_location), rather than
read-md.h's struct file_location, so we'd need to start using libcpp
from the generator programs, porting the location tracking to using
libcpp (e.g. creating linemaps for the files).

There would also be various Makefile.in tweaking to build various files
twice; hopefully that wouldn't lead to any unexpected issues.

Quoting from:
  https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00648.html

> There seem to be two ways to do this:
>
>   (A) build the "light" diagnostics system (errors.c) for the host as
> well as build machine, and link it with the RTL reader there, so
there
> are two parallel diagnostics subsystems.
>
>   (B) build the "real" diagnostics system (diagnostics*) for the
> *build* machine as well as the host, and use it from the gen* tools,
> eliminating the "light" system, and porting the gen* tools to use
> libcpp for location tracking.
>
> Approach (A) seems to be simpler, which is what this part of the
patch
> does.
>
> I've experimented with approach (B).  I think it's doable, but it's
> much more invasive (perhaps needing a libdiagnostics.a and a
> build/libdiagnostics.a in gcc/Makefile.in), so I hope this can be
> followup work.
>
> I can split the relevant parts out into a separate patch, but I was
> wondering if either of you had a strong opinion on (A) vs (B) before
I
> do so?

This patch implements approach (A).

Would you prefer that I went with approach (B), or is approach (A)
acceptable?

Thanks
Dave

On 11/29/2016 07:53 PM, David Malcolm wrote:

> Would you prefer that I went with approach (B), or is approach (A)
> acceptable?

Well, I was hoping there'd be an approach (C) where the read-rtl code 
uses whatever diagnostics framework that is available. Maybe it'll turn 
out that's too hard. Somehow the current patch looked strange to me, but 
if there's no easy alternative maybe we'll have to go with it.


Bernd

On 11/29/2016 10:13 PM, Bernd Schmidt wrote:
> On 11/29/2016 07:53 PM, David Malcolm wrote:
>
>> Would you prefer that I went with approach (B), or is approach (A)
>> acceptable?
>
> Well, I was hoping there'd be an approach (C) where the read-rtl code
> uses whatever diagnostics framework that is available. Maybe it'll turn
> out that's too hard. Somehow the current patch looked strange to me, but
> if there's no easy alternative maybe we'll have to go with it.

So, I've tried to build patches 1-6 + 8, without #7. It looks like the 
differences are as follows:

- A lack of seen_error in errors.[ch], could be easily added, and
   basically a spelling mismatch between have_error and errorcount.
- A lack of fatal in diagnostics.c. Could maybe be added to just call
   fatal_error?

All this seems simpler and cleaner to fix than linking two different 
error handling frameworks into one binary. Do you see any other 
difficulties?


Bernd