| Message ID | 1291979498-1604-5-git-send-email-dodji@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/10/2010 12:11 PM, Dodji Seketeli wrote: > [dodji@adjoa gcc]$ ./cc1 -quiet -ftrack-macro-expansion test.c > test.c: In function ‘g’: > test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) > In macro 'OPERATE' at test.c:2:9 > Expanded at test.c:5:3 > In macro 'SHIFTL' at test.c:5:14 > Expanded at test.c:8:3 > In macro 'MULT' at test.c:8:3 > Expanded at test.c:13:3 > I'm not sure I share Jeff's doubts about the location to present. Possibly _this_ could be controlled by a flag, though. Also, I know this is just an RFC, but the error message should probably look like either this example: test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) test.c:2:9: note: in expansion of macro 'OPERATE' test.c:5:3: note: expanded from here test.c:5:14: note: in expansion of macro 'SHIFTL' test.c:8:3: note: expanded from here test.c:8:3: note: in expansion of macro 'MULT' test.c:13:3: note: expanded from here or this shorter example: test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) test.c:2:9: note: while expanding macro 'OPERATE' test.c:5:14: note: while expanding macro 'SHIFTL' test.c:8:3: note: while expanding macro 'MULT' test.c:13:3: note: expanded from here or this that does not change the location compared to current GCC: test.c:13:3: error: invalid operands to binary<< (have ‘double’ and ‘int’) test.c:2:9: note: while expanding macro 'OPERATE' test.c:5:14: note: while expanding macro 'SHIFTL' test.c:8:3: note: while expanding macro 'MULT' Paolo
On 13 December 2010 15:44, Paolo Bonzini <bonzini@gnu.org> wrote: > On 12/10/2010 12:11 PM, Dodji Seketeli wrote: >> [dodji@adjoa gcc]$ ./cc1 -quiet -ftrack-macro-expansion test.c >> test.c: In function ‘g’: >> test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) >> In macro 'OPERATE' at test.c:2:9 >> Expanded at test.c:5:3 >> In macro 'SHIFTL' at test.c:5:14 >> Expanded at test.c:8:3 >> In macro 'MULT' at test.c:8:3 >> Expanded at test.c:13:3 >> > > > or this shorter example: > > test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) > test.c:2:9: note: while expanding macro 'OPERATE' > test.c:5:14: note: while expanding macro 'SHIFTL' > test.c:8:3: note: while expanding macro 'MULT' > test.c:13:3: note: expanded from here > This format is closer to what GCC currently prints for templates instantiations, however, in template instantiations, the context goes first and the error/warning goes last. The context is also not marked with "note:". See http://people.redhat.com/bkoz/diagnostics/diagnostics.html#template-instantiate-1 and http://people.redhat.com/bkoz/diagnostics/diagnostics.html#9335 and other examples therein. See gcc/cp/error.c:print_instantiation_partial_context, which also handles eliding excessive number of instantiation contexts (too deep macro expansion). I am not saying that this has to be done in this patch series, but it should eventually be implemented, so why not copy what is already available? > or this that does not change the location compared to current GCC: > > test.c:13:3: error: invalid operands to binary<< (have ‘double’ and ‘int’) > test.c:2:9: note: while expanding macro 'OPERATE' > test.c:5:14: note: while expanding macro 'SHIFTL' > test.c:8:3: note: while expanding macro 'MULT' Interestingly, this is closer to the format adopted by Clang (http://clang.llvm.org/diagnostics.html), which I guess just followed GCC. However, I don't see how changing the locations to the ones proposed by Dodji causes any kind of "havoc" for users. In fact, looking at the caret information of clang, the diagnostic would be clearer if the locations were exchanged, which is what Dodji proposes. I don't think gcc diagnostics can be used to do any kind of automatic rewriting. At most, it is parsed in order to present the location of errors/messages in a nicer way, and the new locations proposed by Dodji do not break that. The question should be what locations are clearer/more informative to the user. I think the new locations proposed by Dodji are more informative and they follow what g++ already does for template errors. In any case, I guess that if there is a flag to disable the tracking of macro expansions for memory concerns, the same flag disables the enhanced locations and reverts to the current status, no? No need for yet another separate flag to control the output. Cheers, Manuel.
Paolo Bonzini <bonzini@gnu.org> writes: > On 12/10/2010 12:11 PM, Dodji Seketeli wrote: >> [dodji@adjoa gcc]$ ./cc1 -quiet -ftrack-macro-expansion test.c >> test.c: In function ‘g’: >> test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) >> In macro 'OPERATE' at test.c:2:9 >> Expanded at test.c:5:3 >> In macro 'SHIFTL' at test.c:5:14 >> Expanded at test.c:8:3 >> In macro 'MULT' at test.c:8:3 >> Expanded at test.c:13:3 >> > > I'm not sure I share Jeff's doubts about the location to present. > Possibly _this_ could be controlled by a flag, though. > > Also, I know this is just an RFC, but the error message should > probably look like either this example: > > test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) > test.c:2:9: note: in expansion of macro 'OPERATE' > test.c:5:3: note: expanded from here > test.c:5:14: note: in expansion of macro 'SHIFTL' > test.c:8:3: note: expanded from here > test.c:8:3: note: in expansion of macro 'MULT' > test.c:13:3: note: expanded from here Sorry if I am dig into design consideration when we are talking about UI, but I think it is needed to clarify things here. The current custom in the compiler is that the "<locus> note:" prefix is displayed only when client code explicitely calls the 'inform' function. That sets the diagnostic kind to DK_NOTE. This seems a little bit different from what is done for macro expansion contexts. The macro expansion context is unwound implicitly. That is, the client code calls e.g: error_at (some_location, "An error occured") and if some_location appears to be the location of a token resulting from macro expansion the diagnostic machinery unwinds the expansion context and displays it to the user, regardless of what the diagnostic kind was. So the context lines are not prefixed with "<locus> note:" as the context is generated implicitely. This is similar to what G++ does when it displays template instantiation contexts. On the other hand, I find the "<locus> note:" usage more regular and thus easier to parse for tools that interact with the output of the compiler. So there could be some value in us using that prefix. However if we are to use that prefix for implicitely displayed contexts, I think we should factorize diagnostic_build_prefix to allow using the same prefix as the one used for e.g. DK_NOTE. Would that be acceptable?
Paolo Bonzini <bonzini@gnu.org> writes: > > test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) > test.c:2:9: note: while expanding macro 'OPERATE' > test.c:5:14: note: while expanding macro 'SHIFTL' > test.c:8:3: note: while expanding macro 'MULT' > test.c:13:3: note: expanded from here > In the absence of caret diagnostic I think it is useful to explicitely make the difference between a location in the definition of the macro, and the location of the macro expansion point, i.e: In this error message: test.c: In function ‘g’: test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’) In macro 'OPERATE' at test.c:2:9 Expanded at test.c:5:3 [...] Here is what we are trying to show: 1 #define OPERATE(OPRD1, OPRT, OPRD2) \ 2 OPRD1 OPRT OPRD2; ^ | ---< location in the /definition/ of OPERATE pointed to by: "In macro OPERATE at test.c:2:9" 3 4 #define SHIFTL(A,B) \ 5 OPERATE (A,<<,B) ^ | ---< location of /expansion point/ of OPERATE pointed to by "Expanded at test.c:5:3" [...] I think that's more explicit (and informative) than just saying: > test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) > test.c:2:9: note: while expanding macro 'OPERATE' > test.c:5:14: note: while expanding macro 'SHIFTL' But when we have caret diagnostic where we actually show the user the code we are talking about we can use the "while expanding" just fine.
On Tue, Dec 14, 2010 at 12:54 AM, Dodji Seketeli <dodji@redhat.com> wrote: > The macro expansion context is unwound implicitly. That is, the client > code calls e.g: > > error_at (some_location, "An error occured") > > and if some_location appears to be the location of a token resulting > from macro expansion the diagnostic machinery unwinds the expansion > context and displays it to the user, regardless of what the diagnostic > kind was. So the context lines are not prefixed with "<locus> note:" as > the context is generated implicitely. This is similar to what G++ does > when it displays template instantiation contexts. Let me add some background about this aspect of the diagnostic machinery. The functions inform(), error(), error_at(), etc. are offered as some `high-level' building blocks for constructing more advanced diagnostic functions. It is just that we have not been very disciplined at factorizing things correctly, and instead we tend to go for the easier road of `inlining' calls to error() or inform(). However, models to look for are print_candidates(), cxx_print_error_function, the newly introduced qualified_name_lookup_errorr, etc. The point here is that I would expect CPP to define its own error print diagnostic function that tracks macro expansion context (at bit like what what we do with template instantiation contexts) and combine calls to error_at() and inform(). Note also that we don't capitalize diagnostic messages (and they don't end with periods either.) -- Gaby
On Tue, Dec 14, 2010 at 1:19 AM, Dodji Seketeli <dodji@redhat.com> wrote: > Paolo Bonzini <bonzini@gnu.org> writes: > >> >> test.c:5:14: error: invalid operands to binary<< (have ‘double’ and ‘int’) >> test.c:2:9: note: while expanding macro 'OPERATE' >> test.c:5:14: note: while expanding macro 'SHIFTL' >> test.c:8:3: note: while expanding macro 'MULT' >> test.c:13:3: note: expanded from here >> > > In the absence of caret diagnostic I think it is useful to explicitely > make the difference between a location in the definition of the macro, > and the location of the macro expansion point, i.e: Agreed. But I also agree with Paolo's observation: the prefix "<locus>: note" has to precede the diagnostic message. I know there are other compilers that do it differently, but GCC convention has been to precede diagnostics with loci -- until we move to 2-dimensional diagnostic display (what is sometimes referred to as diagnostics with carets). -- Gaby
On Tue, Dec 14, 2010 at 08:28, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote: > Agreed. But I also agree with Paolo's observation: the prefix "<locus>: note" > has to precede the diagnostic message. I know there are other compilers > that do it differently, but GCC convention has been to precede diagnostics > with loci That was my main point, yes. I'm not sure about the verboseness of having two lines per level of macro expansion, but I can live with that and it was secondary in my message. Paolo
Gabriel Dos Reis <gdr@integrable-solutions.net> writes: > Let me add some background about this aspect of the diagnostic > machinery. The functions inform(), error(), error_at(), etc. are > offered as some `high-level' building blocks for constructing more > advanced diagnostic functions. It is just that we have not been > very disciplined at factorizing things correctly, and instead we > tend to go for the easier road of `inlining' calls to error() or > inform(). Okay. Thank you for this background. > However, models to look for are print_candidates(), > cxx_print_error_function, the newly introduced > qualified_name_lookup_errorr, etc. I see. Though, some of these functions call below the level error and inform by calling pp_base_set_prefix, pp_verbatim and the like. Are those pp_* calls intended as well? > > The point here is that I would expect CPP to define its own error print > diagnostic function that tracks macro expansion context > (at bit like what what we do with template instantiation contexts) > and combine calls to error_at() and inform(). Okay, so I guess I will move the macro expansion unwinder into CPP and make it and use the CPP diagnostic routines that in turn use c_cpp_error (via the cpp_reader callbacks) that is at the same level as inform() error() etc. I will still have to make default_diagnostic_finalizer call that unwinder to make macro expansion context be printed implicitely, though. > Note also that we don't capitalize diagnostic messages (and they don't end > with periods either.) Thanks. I'll fix that.
Paolo Bonzini <bonzini@gnu.org> writes: > On Tue, Dec 14, 2010 at 08:28, Gabriel Dos Reis > <gdr@integrable-solutions.net> wrote: >> Agreed. But I also agree with Paolo's observation: the prefix "<locus>: note" >> has to precede the diagnostic message. I know there are other compilers >> that do it differently, but GCC convention has been to precede diagnostics >> with loci > > That was my main point, yes. I'm not sure about the verboseness of > having two lines per level of macro expansion, but I can live with > that and it was secondary in my message. OK, this is consistent with the discussion we've just had about re-using functions like inform() in another sub-thread. I'll do that then. Thanks.
On Tue, Dec 14, 2010 at 2:22 AM, Dodji Seketeli <dodji@redhat.com> wrote: >> However, models to look for are print_candidates(), >> cxx_print_error_function, the newly introduced >> qualified_name_lookup_errorr, etc. > > I see. Though, some of these functions call below the level error and > inform by calling pp_base_set_prefix, pp_verbatim and the like. Are > those pp_* calls intended as well? Yes, that is another dimension to the diagnostic machinery. The idea here is that every client (i.e. front-end) may have som front-end specific actions to take (e.g. printing template instantiation contexts) before the core diagnostic message is printed. That is the role of `diagnostic starter' functions (see the comments in diagnostic.h). Similarly we have `diagnostic finalizer' functions which are supposed to do any sort of `cleanup' after a diagnostic is printed. Now, you can consider CPP as a sort of front-end that turns raw input file into a stream of tokens which is then handed over to the C or C++ parsers -- but I'm not sure that is the way it currently works. So, another option is to find a combination of these two dimensions. > >> >> The point here is that I would expect CPP to define its own error print >> diagnostic function that tracks macro expansion context >> (at bit like what what we do with template instantiation contexts) >> and combine calls to error_at() and inform(). > > Okay, so I guess I will move the macro expansion unwinder into CPP and > make it and use the CPP diagnostic routines that in turn use c_cpp_error > (via the cpp_reader callbacks) that is at the same level as inform() > error() etc. It is my belief that the macro-expansion related diagnostics specific improvements belong there. > > I will still have to make default_diagnostic_finalizer call that > unwinder to make macro expansion context be printed implicitely, though. something you can't do with CPP specific diagnostic finalizer? -- Gaby
Gabriel Dos Reis <gdr@integrable-solutions.net> writes: [...] >> I see. Though, some of these functions call below the level error and >> inform by calling pp_base_set_prefix, pp_verbatim and the like. Are >> those pp_* calls intended as well? > > Yes, that is another dimension to the diagnostic machinery. > The idea here is that every client (i.e. front-end) may have som > front-end specific actions to take (e.g. printing template instantiation > contexts) before the core diagnostic message is printed. That > is the role of `diagnostic starter' functions (see the comments > in diagnostic.h). Similarly we have `diagnostic finalizer' functions > which are supposed to do any sort of `cleanup' after a diagnostic > is printed. OK. And front-ends are supposed to be able to provide their own diagnostics starter and finalizer functions. > Now, you can consider CPP as a sort of front-end that turns > raw input file into a stream of tokens which is then handed over > to the C or C++ parsers -- but I'm not sure that is the way it currently > works. A sort of front-end indeed. [..] >> Okay, so I guess I will move the macro expansion unwinder into CPP and >> make it and use the CPP diagnostic routines that in turn use c_cpp_error >> (via the cpp_reader callbacks) that is at the same level as inform() >> error() etc. > > It is my belief that the macro-expansion related diagnostics specific > improvements belong there. > >> >> I will still have to make default_diagnostic_finalizer call that >> unwinder to make macro expansion context be printed implicitely, though. > > something you can't do with CPP specific diagnostic finalizer? Good catch. The thing is CPP is used like a library by the front-end (C, C++, Fortran) that needs to consume the tokens it generates. As such, CPP itself doesn't override the diagnostic finalizer. It's the front-end that does it. And right now the C and Fortran FEs just use the default diagnostic finalizer. If I underrstand correctly, I should now provide a specific diagnostic finalizer for C and Fortran FEs (like what G++ does) and have them call the macro expansion unwinder (that would be) provided by CPP and leave default_diagnostic_finalizer alone.
On Tue, Dec 14, 2010 at 3:22 AM, Dodji Seketeli <dodji@redhat.com> wrote: > Gabriel Dos Reis <gdr@integrable-solutions.net> writes: > > [...] > >>> I see. Though, some of these functions call below the level error and >>> inform by calling pp_base_set_prefix, pp_verbatim and the like. Are >>> those pp_* calls intended as well? >> >> Yes, that is another dimension to the diagnostic machinery. >> The idea here is that every client (i.e. front-end) may have som >> front-end specific actions to take (e.g. printing template instantiation >> contexts) before the core diagnostic message is printed. That >> is the role of `diagnostic starter' functions (see the comments >> in diagnostic.h). Similarly we have `diagnostic finalizer' functions >> which are supposed to do any sort of `cleanup' after a diagnostic >> is printed. > > OK. And front-ends are supposed to be able to provide their own > diagnostics starter and finalizer functions. yes. [..] >>> I will still have to make default_diagnostic_finalizer call that >>> unwinder to make macro expansion context be printed implicitely, though. >> >> something you can't do with CPP specific diagnostic finalizer? > > Good catch. The thing is CPP is used like a library by the front-end (C, > C++, Fortran) that needs to consume the tokens it generates. As such, > CPP itself doesn't override the diagnostic finalizer. It's the front-end > that does it. And right now the C and Fortran FEs just use the default > diagnostic finalizer. > > If I underrstand correctly, I should now provide a specific diagnostic > finalizer for C and Fortran FEs (like what G++ does) and have them call > the macro expansion unwinder (that would be) provided by CPP and leave > default_diagnostic_finalizer alone. yes, that sounds good.
diff --git a/gcc/Makefile.in b/gcc/Makefile.in index 717326c..604f626 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -2825,7 +2825,7 @@ fold-const.o : fold-const.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) \ $(GGC_H) $(TM_P_H) langhooks.h $(MD5_H) intl.h $(TARGET_H) \ $(GIMPLE_H) realmpfr.h $(TREE_FLOW_H) diagnostic.o : diagnostic.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \ - version.h $(INPUT_H) intl.h $(DIAGNOSTIC_H) diagnostic.def + version.h $(INPUT_H) intl.h $(DIAGNOSTIC_H) diagnostic.def $(VEC_H) opts.o : opts.c $(OPTS_H) $(OPTIONS_H) $(DIAGNOSTIC_CORE_H) $(CONFIG_H) $(SYSTEM_H) \ coretypes.h $(TM_H) $(RTL_H) \ $(DIAGNOSTIC_H) $(INSN_ATTR_H) intl.h $(TARGET_H) \ diff --git a/gcc/cp/error.c b/gcc/cp/error.c index ed168c4..db35c61 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -2663,7 +2663,7 @@ static void cp_diagnostic_starter (diagnostic_context *context, diagnostic_info *diagnostic) { - diagnostic_report_current_module (context); + diagnostic_report_current_module (context, diagnostic->location); cp_print_error_function (context, diagnostic); maybe_print_instantiation_context (context); maybe_print_constexpr_context (context); diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c index 9df540b..671b6ea 100644 --- a/gcc/diagnostic.c +++ b/gcc/diagnostic.c @@ -30,6 +30,15 @@ along with GCC; see the file COPYING3. If not see #include "input.h" #include "intl.h" #include "diagnostic.h" +#include "vec.h" + +typedef struct +{ + const struct line_map *map; + source_location where; +} loc_t; +DEF_VEC_O(loc_t); +DEF_VEC_ALLOC_O (loc_t,heap); #define pedantic_warning_kind(DC) \ ((DC)->pedantic_errors ? DK_ERROR : DK_WARNING) @@ -45,6 +54,9 @@ static void diagnostic_action_after_output (diagnostic_context *, diagnostic_info *); static void real_abort (void) ATTRIBUTE_NORETURN; +static void unwind_expanded_macro_location (diagnostic_context *, source_location, + const struct line_map **); + /* Name of program invoked, sans directories. */ const char *progname; @@ -254,10 +266,138 @@ diagnostic_action_after_output (diagnostic_context *context, } } +/* Unwind the different macro expansions that lead to the token which + location is WHERE and emit diagnostics showing the resulting + unwound macro expansion stack. If TOPMOST_EXP_POINT_MAP is + non-null, *TOPMOST_EXP_POINT_MAP is set to the map of the + expansion point of the top most macro of the stack. This must be + an ordinary map. */ + +static void +unwind_expanded_macro_location (diagnostic_context *context, + source_location where, + const struct line_map **topmost_exp_point_map) +{ + const struct line_map *map, *resolved_map; + bool unwind = true; + source_location resolved_location; + VEC(loc_t, heap) *loc_vec; + unsigned ix, len; + loc_t loc, *iter; + + map = linemap_lookup (line_table, where); + if (!linemap_macro_expansion_map_p (map)) + return; + + loc_vec = VEC_alloc (loc_t, heap, 4); + + /* Let's unwind the stack of macros that got expanded and that led + to the token which location is WHERE. We are going to store the + stack into MAP_VEC, so that we can later walk MAP_VEC backward to + display a somewhat meaningful trace of the macro expansion + history to the user. Note that the deepest macro expansion is + going to be store at the beginning of MAP_VEC. */ + while (unwind) + { + loc.where = where; + loc.map = map; + VEC_safe_push (loc_t, heap, loc_vec, &loc); + + /* WHERE is the location of a token inside the expansion of a + macro. MAP is the map holding the locations of that macro + expansion. Let's get the location of the token inside the + *definition* of the macro of MAP, that got expanded at + WHERE. This is basically how we go "up" in the stack of + macro expansions that led to WHERE. */ + resolved_location = + linemap_macro_map_loc_to_def_point (map, where, false); + resolved_map = linemap_lookup (line_table, resolved_location); + + /* If the token at RESOLVED_LOCATION [at macro definition point] + is itself inside an expanded macro then we keep unwinding the + expansion stack by tracing the "parent macro" that got expanded + inside the definition of the macro of MAP... */ + if (linemap_macro_expansion_map_p (resolved_map)) + { + where = resolved_location; + map = resolved_map; + } + else + { + /* Otherwise, let's consider the location of the expansion + point of the macro of MAP. Keep in mind that MAP is a + macro expansion map. To get a "normal map" (i.e a non + macro expansion map) and be done with the unwinding, we + must either consider the location of the location + expansion point of the macro or the location of the token + inside the macro definition that got expanded to + WHERE. */ + where = + linemap_macro_map_loc_to_exp_point (map, where); + map = linemap_lookup (line_table, where); + } + if (!linemap_macro_expansion_map_p (map)) + unwind = false; + } + + if (topmost_exp_point_map) + *topmost_exp_point_map = map; + + /* Walk the map_vec and print the macro expansion stack. */ + len = VEC_length (loc_t, loc_vec); + for (ix = 0; + len && ix < len; + ++ix) + { + expanded_location def_loc, exp_loc; + const struct line_map *def_point_map = NULL, + *exp_point_map = NULL; + + iter = VEC_index (loc_t, loc_vec, ix); + + /* Okay, now here is what we want. For each token resulting + from macro expansion we want to show: + 1/ where in the definition of the macro the token comes from. + + 2/ where the macro got expanded. */ + + /* Expand the location iter->where into the locus 1/ of the + comment above. */ + def_loc = + linemap_expand_location_full (line_table, iter->where, + LRK_MACRO_PARM_REPLACEMENT_POINT, + &def_point_map); + + /* Expand the location of the expansion point of the macro + which expansion gave the token at represented by + def_loc. This is the locus 2/ of the earlier comment. */ + exp_loc = + linemap_expand_location_full (line_table, + MACRO_MAP_EXPANSION_POINT_LOCATION + (iter->map), + LRK_MACRO_PARM_REPLACEMENT_POINT, + &exp_point_map); + + if (!LINEMAP_SYSP (resolved_map)) + { + pp_verbatim (context->printer, + "\nIn macro '%s' at %s:%d:%d", + linemap_map_get_macro_name (iter->map), + LINEMAP_FILE (def_point_map), + def_loc.line, def_loc.column); + pp_verbatim (context->printer, + "\n Expanded at %s:%d:%d", + LINEMAP_FILE (exp_point_map), + exp_loc.line, exp_loc.column); + } + } + VEC_free (loc_t, heap, loc_vec); +} + void -diagnostic_report_current_module (diagnostic_context *context) +diagnostic_report_current_module (diagnostic_context *context, location_t where) { - const struct line_map *map; + const struct line_map *map = NULL; if (pp_needs_newline (context->printer)) { @@ -265,10 +405,13 @@ diagnostic_report_current_module (diagnostic_context *context) pp_needs_newline (context->printer) = false; } - if (input_location <= BUILTINS_LOCATION) + if (where <= BUILTINS_LOCATION) return; - map = linemap_lookup (line_table, input_location); + linemap_expand_location_full (line_table, where, + LRK_MACRO_PARM_REPLACEMENT_POINT, + &map); + if (map && diagnostic_last_module_changed (context, map)) { diagnostic_set_last_module (context, map); @@ -301,7 +444,7 @@ void default_diagnostic_starter (diagnostic_context *context, diagnostic_info *diagnostic) { - diagnostic_report_current_module (context); + diagnostic_report_current_module (context, diagnostic->location); pp_set_prefix (context->printer, diagnostic_build_prefix (context, diagnostic)); } @@ -310,6 +453,7 @@ void default_diagnostic_finalizer (diagnostic_context *context, diagnostic_info *diagnostic ATTRIBUTE_UNUSED) { + unwind_expanded_macro_location (context, diagnostic->location, NULL); pp_destroy_prefix (context->printer); } diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h index 8074354..4b1265b 100644 --- a/gcc/diagnostic.h +++ b/gcc/diagnostic.h @@ -253,7 +253,7 @@ extern diagnostic_context *global_dc; /* Diagnostic related functions. */ extern void diagnostic_initialize (diagnostic_context *, int); extern void diagnostic_finish (diagnostic_context *); -extern void diagnostic_report_current_module (diagnostic_context *); +extern void diagnostic_report_current_module (diagnostic_context *, location_t); /* Force diagnostics controlled by OPTIDX to be kind KIND. */ extern diagnostic_t diagnostic_classify_diagnostic (diagnostic_context *, diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist15.C b/gcc/testsuite/g++.dg/cpp0x/initlist15.C index b75cc81..cca56b1 100644 --- a/gcc/testsuite/g++.dg/cpp0x/initlist15.C +++ b/gcc/testsuite/g++.dg/cpp0x/initlist15.C @@ -2,6 +2,7 @@ // Just discard errors pointing at header files // { dg-prune-output "include" } +// { dg-prune-output " from" } #include <vector> #include <typeinfo> diff --git a/gcc/testsuite/g++.old-deja/g++.robertl/eb43.C b/gcc/testsuite/g++.old-deja/g++.robertl/eb43.C index 1dc4328..bd784b1 100644 --- a/gcc/testsuite/g++.old-deja/g++.robertl/eb43.C +++ b/gcc/testsuite/g++.old-deja/g++.robertl/eb43.C @@ -6,6 +6,10 @@ // { dg-prune-output "note" } +// Discard errors pointing at header files +// { dg-prune-output "In file included from" } +// { dg-prune-output " from" } + #include <vector> #include <algorithm> #include <functional> diff --git a/gcc/testsuite/g++.old-deja/g++.robertl/eb79.C b/gcc/testsuite/g++.old-deja/g++.robertl/eb79.C index 1c1ad3e..60cc713 100644 --- a/gcc/testsuite/g++.old-deja/g++.robertl/eb79.C +++ b/gcc/testsuite/g++.old-deja/g++.robertl/eb79.C @@ -1,5 +1,9 @@ // { dg-do assemble } // { dg-prune-output "note" } + +// Discard errors pointing at header files +// { dg-prune-output "In file included from" } +// { dg-prune-output " from" } // Makes bogus x86 assembly code. #include <iostream> diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c new file mode 100644 index 0000000..a087050 --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c @@ -0,0 +1,30 @@ +/* + { dg-options "-ftrack-macro-expansion=1" } + { dg-do compile } +*/ + +#define OPERATE(OPRD1, OPRT, OPRD2) \ +do \ +{ \ + OPRD1 OPRT OPRD2; \ +} while (0) + +#define SHIFTL(A,B) \ + OPERATE (A,<<,B) /* { dg-error "invalid operands to binary <<" } */ + +void +foo () +{ + SHIFTL (0.1,0.2); +} + +/* + { dg-message "macro 'OPERATE'\[^\n\r\]*:9:8" "In macro OPERATE" { target *-*-* } 0 } +{ dg-message "Expanded at\[^\n\r\]*:13:3" "OPERATE expansion point" { target *-*-* } 0 } + + { dg-message "macro 'SHIFTL'\[^\n\r\]*:13:14" "In macro SHIFTL" { target *-*-* } 0 } + +{ dg-message "Expanded at\[^\n\r\]*:18:3" "SHIFTL expansion point" { target *-*-* } 0 } + + +*/ diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c new file mode 100644 index 0000000..0f15875 --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c @@ -0,0 +1,31 @@ +/* + { dg-options "-ftrack-macro-expansion=1" } + { dg-do compile } +*/ + +#define OPERATE(OPRD1, OPRT, OPRD2) \ + OPRD1 OPRT OPRD2; + +#define SHIFTL(A,B) \ + OPERATE (A,<<,B) /* { dg-error "invalid operands to binary <<" } */ + +#define MULT(A) \ + SHIFTL (A,1) + +void +foo () +{ + MULT (1.0);/* 1.0 << 1;*/ +} + +/* + { dg-message "macro 'OPERATE'\[^\n\r\]*:7:8*" "In macro OPERATE" { target *-*-* } 0 } + { dg-message "Expanded at\[^\n\r\]*:10:3" "OPERATE expansion point" { target *-*-* } 0 } + + { dg-message "macro 'SHIFTL'\[^\n\r\]*:10:14" "In macro SHIFTL" { target *-*-* } 0 } +{ dg-message "Expanded at\[^\n\r\]*:13:3" "SHIFTL expansion point" { target *-*-* } 0 } + + { dg-message "macro 'MULT'\[^\n\r\]*:13:3" "In macro MULT" { target *-*-* } 0 } +{ dg-message "Expanded at\[^\n\r\]*:18:3" "MULT expansion point" { target *-*-* } 0 } + + */ diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c new file mode 100644 index 0000000..0b5c662 --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c @@ -0,0 +1,18 @@ +/* + { dg-options "-fshow-column -ftrack-macro-expansion=1" } + { dg-do compile } + */ + +#define SQUARE(A) A * A + +void +foo() +{ + SQUARE (1 << 0.1); /* { dg-error "16:invalid operands to binary <<" } */ +} + +/* +{ dg-message "macro 'SQUARE'\[^\n\r\]*:6:19" "In macro SQUARE" { target *-*-* } 0 } +{ dg-message "Expanded at\[^\n\r\]*:11:3" "SQUARE expansion point" { target *-*-* } 0 } + +*/ diff --git a/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c new file mode 100644 index 0000000..f9e4c2d --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c @@ -0,0 +1,19 @@ +/* + { dg-options "-fshow-column -ftrack-macro-expansion=2" } + { dg-do compile } + */ + +#define SQUARE(A) A * A + +void +foo() +{ + SQUARE (1 << 0.1); /* { dg-error "13:invalid operands to binary <<" } */ +} + +/* + +{ dg-message "macro 'SQUARE'\[^\n\r\]*:6:19" "In macro SQUARE" { target *-*-* } 0 } +{ dg-message "Expanded at\[^\n\r\]*:11:3" "SQUARE expansion point" { target *-*-* } 0 } + +*/ diff --git a/gcc/testsuite/gcc.target/i386/sse-vect-types.c b/gcc/testsuite/gcc.target/i386/sse-vect-types.c index 9cb6f3e..ce70125 100644 --- a/gcc/testsuite/gcc.target/i386/sse-vect-types.c +++ b/gcc/testsuite/gcc.target/i386/sse-vect-types.c @@ -1,6 +1,12 @@ /* { dg-do compile } */ /* { dg-options "-O0 -msse2" } */ + +/* + Just discard diagnostic prolog about errors in include files + { dg-prune-output "In file included from" } +*/ + #include <xmmintrin.h> __m128d foo1(__m128d z, __m128d a, int N) { diff --git a/gcc/tree-diagnostic.c b/gcc/tree-diagnostic.c index b456a2a..cbfd81c 100644 --- a/gcc/tree-diagnostic.c +++ b/gcc/tree-diagnostic.c @@ -35,7 +35,7 @@ void diagnostic_report_current_function (diagnostic_context *context, diagnostic_info *diagnostic) { - diagnostic_report_current_module (context); + diagnostic_report_current_module (context, diagnostic->location); lang_hooks.print_error_function (context, input_filename, diagnostic); }
In this third instalment the diagnostic machinery -- when faced with the virtual location of a token resulting from macro expansion -- uses the new linemap APIs to unwind the stack of macro expansions that led to that token and emits a [hopefully] more useful message than what we have today. diagnostic_report_current_module has been slightly changed to use the location given by client code instead of the global input_location variable. This results in more precise diagnostic locations in general but then the patch adjusts some C++ tests which output changed as a result of this. Three new regression tests have been added. The mandatory screenshot goes like this: [dodji@adjoa gcc]$ cat -n test.c 1 #define OPERATE(OPRD1, OPRT, OPRD2) \ 2 OPRD1 OPRT OPRD2; 3 4 #define SHIFTL(A,B) \ 5 OPERATE (A,<<,B) 6 7 #define MULT(A) \ 8 SHIFTL (A,1) 9 10 void 11 g () 12 { 13 MULT (1.0);/* 1.0 << 1; <-- so this is an error. */ 14 } [dodji@adjoa gcc]$ ./cc1 -quiet -ftrack-macro-expansion test.c test.c: In function ‘g’: test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’) In macro 'OPERATE' at test.c:2:9 Expanded at test.c:5:3 In macro 'SHIFTL' at test.c:5:14 Expanded at test.c:8:3 In macro 'MULT' at test.c:8:3 Expanded at test.c:13:3 The combination of this patch and the previous ones boostrapped with --enable-languages=all,ada and passed regression tests on x86_64-unknown-linux-gnu. gcc/ * gcc/diagnostic.h (diagnostic_report_current_module): Add a location parameter. * diagnostic.c (struct loc_t): new struct. (diagnostic_report_current_module): Add a location parameter to the function definition. Use it here instead of input_location. Fully expand the location rather than just looking up its map and risking to touch a resulting macro map. (default_diagnostic_starter): Pass the relevant diagnostic location to diagnostic_report_current_module. (unwind_expanded_macro_location): New function. (default_diagnostic_finalizer): Use it. * tree-diagnostic.c (diagnostic_report_current_function): Pass the relevant location to diagnostic_report_current_module. gcc/cp/ * error.c (cp_diagnostic_starter): Pass the relevant location to diagnostic_report_current_module. gcc/testsuite/ * gcc.dg/cpp/macro-exp-tracking-1.c: New test. * gcc.dg/cpp/macro-exp-tracking-2.c: Likewise. * gcc.dg/cpp/macro-exp-tracking-3.c: Likewise. * g++.dg/cpp0x/initlist15.C: Discard errors pointing at multiple levels of included files. * g++.old-deja/g++.robertl/eb43.C: Likewise. * g++.old-deja/g++.robertl/eb79.C: Likewise. * gcc.target/i386/sse-vect-types.c: Likewise. --- gcc/Makefile.in | 2 +- gcc/cp/error.c | 2 +- gcc/diagnostic.c | 154 ++++++++++++++++++++++- gcc/diagnostic.h | 2 +- gcc/testsuite/g++.dg/cpp0x/initlist15.C | 1 + gcc/testsuite/g++.old-deja/g++.robertl/eb43.C | 4 + gcc/testsuite/g++.old-deja/g++.robertl/eb79.C | 4 + gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c | 30 +++++ gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c | 31 +++++ gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c | 18 +++ gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c | 19 +++ gcc/testsuite/gcc.target/i386/sse-vect-types.c | 6 + gcc/tree-diagnostic.c | 2 +- 13 files changed, 266 insertions(+), 9 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-1.c create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-2.c create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-3.c create mode 100644 gcc/testsuite/gcc.dg/cpp/macro-exp-tracking-4.c