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

[Bernd Schmidt]

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

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

[Bernd Schmidt]

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

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

[David Malcolm]

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

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

[Bernd Schmidt]

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

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

[David Malcolm]

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.

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

[Bernd Schmidt]

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

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

[Richard Sandiford]

David Malcolm  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

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

[David Malcolm]

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  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(-)

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index fa54215..c265893 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1266,6 +1266,7 @@ OBJS = \
dwarf2cfi.o \
dwarf2out.o \
emit-rtl.o \
+   errors.o \
et-forest.o \
except.o \
explow.o \
diff --git a/gcc/errors.c b/gcc/errors.c
index 468384c..8df94ab 100644
--- a/gcc/errors.c
+++ b/gcc/errors.c
@@ -21,18 +21,21 @@ along with GCC; see the file COPYING3.  If not see
in the generator programs; the compiler has a more elaborate suite
of diagnostic printers, found in diagnostic.c.  */
 
-#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"
 
 /* Set this to argv[0] at the beginning of main.  */
 
+#ifdef GENERATOR_FILE
 const char *progname;
+#endif /* #ifdef GENERATOR_FILE */
 
 /* Starts