Message ID | 1478898935-46932-8-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
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
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 out 0, set to 1 if error is called. */ @@ -55,13 +58,15 @@ warning (const char *format, ...) /* Print an error message - we keep going but the output is unusable. */ +#ifdef GENERATOR_FILE + void error (const char *format, ...) { va_list ap; va_start (ap, format); - fprintf (stderr, "%s: ", progname); + fprintf (stderr, "%s: error: ", progname); vfprintf (stderr, format, ap); va_end (ap); fputc ('\n', stderr); @@ -69,6 +74,7 @@ error (const char *format, ...) have_error = 1; } +#endif /* #ifdef GENERATOR_FILE */ /* Fatal error - terminate execution immediately. Does not return. */ @@ -78,7 +84,7 @@ fatal (const char *format, ...) va_list ap; va_start (ap, format); - fprintf (stderr, "%s: ", progname); + fprintf (stderr, "%s: error: ", progname); vfprintf (stderr, format, ap); va_end (ap); fputc ('\n', stderr); @@ -87,6 +93,8 @@ fatal (const char *format, ...) /* Similar, but say we got an internal error. */ +#ifdef GENERATOR_FILE + void internal_error (const char *format, ...) { @@ -127,8 +135,11 @@ trim_filename (const char *name) /* "Fancy" abort. Reports where in the compiler someone gave up. This file is used only by build programs, so we're not as polite as the version in diagnostic.c. */ + void fancy_abort (const char *file, int line, const char *func) { internal_error ("abort in %s, at %s:%d", func, trim_filename (file), line); } + +#endif /* #ifdef GENERATOR_FILE */ diff --git a/gcc/errors.h b/gcc/errors.h index a6973f3..ebafa77 100644 --- a/gcc/errors.h +++ b/gcc/errors.h @@ -28,8 +28,22 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_ERRORS_H #define GCC_ERRORS_H +/* Records a position in the file. */ +struct file_location { + file_location () {} + file_location (const char *, int, int); + + const char *filename; + int lineno; + int colno; +}; + +inline file_location::file_location (const char *filename_in, int lineno_in, int colno_in) +: filename (filename_in), lineno (lineno_in), colno (colno_in) {} + extern void warning (const char *, ...) ATTRIBUTE_PRINTF_1 ATTRIBUTE_COLD; extern void error (const char *, ...) ATTRIBUTE_PRINTF_1 ATTRIBUTE_COLD; +extern void error_at (file_location, const char *, ...) ATTRIBUTE_PRINTF_2 ATTRIBUTE_COLD; extern void fatal (const char *, ...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_1 ATTRIBUTE_COLD; extern void internal_error (const char *, ...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF_1 ATTRIBUTE_COLD; extern const char *trim_filename (const char *); diff --git a/gcc/read-md.h b/gcc/read-md.h index 27fc9c2..8910b75 100644 --- a/gcc/read-md.h +++ b/gcc/read-md.h @@ -21,19 +21,7 @@ along with GCC; see the file COPYING3. If not see #define GCC_READ_MD_H #include "obstack.h" - -/* Records a position in the file. */ -struct file_location { - file_location () {} - file_location (const char *, int, int); - - const char *filename; - int lineno; - int colno; -}; - -inline file_location::file_location (const char *filename_in, int lineno_in, int colno_in) -: filename (filename_in), lineno (lineno_in), colno (colno_in) {} +#include "errors.h" /* Holds one symbol or number in the .md file. */ struct md_name {