(preprocessed source doesn't show the error due to the system_header pragma) $ cat test.cc #define bar 1 #include "header.h" $ cat header.h #pragma GCC system_header int bar = 0; $ g++ test.cc test.cc:1:13: error: expected unqualified-id before numeric constant #define bar 1 ^ This is no help because it doesn't point you to the line in header.h that clashes with 'bar' Without the system_header pragma you get test.cc:1:13: error: expected unqualified-id before numeric constant #define bar 1 ^ header.h:2:5: note: in expansion of macro 'bar' int bar = 0; ^ Which at least shows both locations, although the first diagnostic is still confusing ... are the two locations backwards? The "expected unqualified-id before numeric constant" should point to "int bar = 0;"
clang++ gets this right, printing the same thing with or without the system_header pragma, and swapping the locations of the error and the note: In file included from test.cc:2: ./header.h:1:5: error: expected unqualified-id int bar = 0; ^ test.cc:1:13: note: expanded from macro 'bar' #define bar 1 ^ 1 error generated. This makes much more sense.
I think this his how the macro expansion was designed to work: It shows the location of the token that triggered the error. I also agree that the clang way makes more sense in this case.
Hum, I am not sure why the macro unwinder avoids unwinding if the macro comes from a system-header. If a warning message comes from a system-header, then it should have been suppressed earlier and never reach the macro unwinder. Otherwise, we already have emitted the diagnostic, so the macro unwinder just provides more context.
On the other hand, let's consider: pr55252.c: #define bar 256 #include "pr55252.h" pr55252.h: #pragma GCC system_header signed char foo = bar; In this case, I would expect the warning to be suppressed (as it is with -ftrack-macro-expansion=0). However, since the warning is actually given in the C file, it is emitted. I am getting more and more convinced that expanding to spelling point might not be the best (I think Paolo Bonzini raised this issue at the time...). On the other hand, this is a very contrived testcase. I wouldn't expect in normal code that the expansion point to be in a system-header and the spelling point in a non-system-header. Dodji, what do you think?
(In reply to comment #4) > On the other hand, this is a very contrived testcase. I > wouldn't expect in normal code that the expansion point to be in a > system-header and the spelling point in a non-system-header. It's not that contrived, here's a more realistic example: #define fixed 1 #include <iostream> int main() { } This gives a very unhelpful error: t.cc:1:15: error: expected unqualified-id before numeric constant #define fixed 1 ^ t.cc:1:15: error: expected unqualified-id before numeric constant #define fixed 1 ^ t.cc:1:15: error: expected initializer before numeric constant
> I think this his how the macro expansion was designed to work: It > shows the location of the token that triggered the error. Yes. And there are cases where the GCC way seems to make more sense; for instance: $ 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 } $ With GCC: ./test.c: In function ‘g’: ./test.c:5:14: error: invalid operands to binary << (have ‘double’ and ‘int’) OPERATE (A,<<,B) ^ ./test.c:2:9: note: in definition of macro ‘OPERATE’ OPRD1 OPRT OPRD2; ^ ./test.c:8:3: note: in expansion of macro ‘SHIFTL’ SHIFTL (A,1) ^ ./test.c:13:3: note: in expansion of macro ‘MULT’ MULT (1.0);// 1.0 << 1; <-- so this is an error. ^ $ With Clang: test.c:13:3: error: invalid operands to binary expression ('double' and 'int') MULT (1.0);// 1.0 << 1; <-- so this is an error. ^~~~~~~~~~ test.c:8:3: note: expanded from macro 'MULT' SHIFTL (A,1) ^ test.c:5:14: note: expanded from macro 'SHIFTL' OPERATE (A,<<,B) ^ test.c:2:9: note: expanded from macro 'OPERATE' OPRD1 OPRT OPRD2; ~~~~~ ^ ~~~~~ 1 error generated. $ So, at line 13.3, I think it doesn't make sense to talk about a binary expression (which is related to the '<<' operator). So yes, we chose to first point to the token on which the error appeared, and then print the context of the macro expansion all the way to the expansion point. Now I am not sure what the best approach should be. > I also agree that the clang way makes more sense in this case. Indeed.
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> a écrit: > On the other hand, let's consider: > pr55252.c: > > #define bar 256 > #include "pr55252.h" > > pr55252.h: > > #pragma GCC system_header > signed char foo = bar; > > > > In this case, I would expect the warning to be suppressed (as it is with > > -ftrack-macro-expansion=0). However, since the warning is actually > given in the C file, it is emitted. I am getting more and more > convinced that expanding to spelling point might not be the best (I > think Paolo Bonzini raised this issue at the time...). I am thinking that a way to really handle this correctly is to have the concept of the location of the "current expression", which the current token belongs to. In this case, we are emitting the warning on the token 'bar' (which is spelled in the C file), while the relevant expression (the variable declaration) is in a header file. The diagnostics machinery should be able to say "is the current expression in a header file?", and act appropriately. Would that make sense in the grand scheme of things?
(In reply to comment #7) > Would that make sense in the grand scheme of things? The idea seems good. It would also handle comment #4 testcase. However, I am not sure how you would implement this. A different issue is why the macro unwinder cares about system-headers? See comment #3.
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> a écrit: > Hum, I am not sure why the macro unwinder avoids unwinding if the > macro comes from a system-header. If a warning message comes from a > system-header, then it should have been suppressed earlier and never > reach the macro unwinder. Otherwise, we already have emitted the > diagnostic, so the macro unwinder just provides more context. Yeah. I agree this is weird. There are cases where the spelling location is in normal main file, but some locations in its macro expansion context are e.g, for built-in tokens. In that case we can get ugly diagnostic prefixes like: <built-in>:0:0: warning: conversion to 'float' alters 'int' constant value This is what the commit r186970 tries to fix. Please read the commit log (that contains useful information about the context of the bug, besides the ChangeLog that we like to put in there) of that revision to understand why we are skipping each locus that comes from a system header too. I guess a way to handle this is to 1/ make the macro unwinder call linemap_unwind_to_first_non_reserved_loc at the beginning of the unwind process and start unwinding from there 2/ during the rest of the unwind process, dismiss reserved locations only. Not those coming from system headers. Would this make sense?
"manu at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org> a écrit: > The idea seems good. It would also handle comment #4 testcase. Yeah, and I think it would be a step in the direction of printing ranges for expressions. I know you and I share a (not so) secret dream of seeing taht in GCC one day. :-) > However, I am not sure how you would implement this. That is the real issue, I guess. I suspect this could be like opening a can of worms. In G++, I guess I'd start with updating the hypothetical 'current expression' variable in cp_parser_expression. But then we need to handle tentative parsing. That is, if we are in a tentative parse, save the new 'current expression' on the side, and really save it when the parse is committed. And pray for the fall-outs to not be in the order of dozens. :-) > different issue is why the macro unwinder cares about system-headers? See > comment #3. Right, I have replied to that in an earlier comment. Thanks.
*** Bug 58953 has been marked as a duplicate of this bug. ***
PR52962 is another case where it would make more sense to give the error on the macro expansion location rather than on the macro definition.
*** Bug 59495 has been marked as a duplicate of this bug. ***
*** Bug 61165 has been marked as a duplicate of this bug. ***
*** Bug 61803 has been marked as a duplicate of this bug. ***
I've tripped across this enough that I've actually filed dups twice now. I think it would be best to change the ordering here. That is, the initial error ought to generally be the location of the outermost expansion. Then, the remaining notes ought to delineate the macro expansions. While it is true that this will yield a sub-optimal result in some cases, I think that it will have better results in the preponderance of cases. That is, there's no way to be perfect here but gcc could be more useful.
It would be less of a pain if -Wsystem-headers caused both locations to be printed, but it doesn't, so sometimes the only option is to dump the preprocessed source without line markers and then compile that to get two locations, then map the location in the preprocessed source back to a line in the original source.
(In reply to Jonathan Wakely from comment #17) > It would be less of a pain if -Wsystem-headers caused both locations to be > printed, but it doesn't, so sometimes the only option is to dump the > preprocessed source without line markers and then compile that to get two > locations, then map the location in the preprocessed source back to a line > in the original source. You could try with this: Index: tree-diagnostic.c =================================================================== --- tree-diagnostic.c (revision 220306) +++ tree-diagnostic.c (working copy) @@ -197,12 +197,14 @@ maybe_unwind_expanded_macro_loc (diagnos within a system header. */ const struct line_map *m = NULL; source_location l = linemap_resolve_location (line_table, resolved_def_loc, LRK_SPELLING_LOCATION, &m); - if (l < RESERVED_LOCATION_COUNT || LINEMAP_SYSP (m)) - continue; + if (l < RESERVED_LOCATION_COUNT) + continue; + if (LINEMAP_SYSP (m) && !context->dc_warn_system_headers) + continue; /* We need to print the context of the macro definition only when the locus of the first displayed diagnostic (displayed before this trace) was inside the definition of the macro. */ .. but I still not see why we need to skip system-headers at all. The comment in the original patch (r186970) I am then using that facility in the diagnostics printing module and in the macro unwinder to avoid printing diagnostics lines that refer to the locations for built-ins or more generally for reserved locations. Note that when I start the dance of skipping a built-in location I also skip locations that are in system headers, because it turned out that a lot of those built-ins are actually used in system headers (e.g, "#define INT_MAX __INT_MAX__" where __INT_MAX__ is a built-in). suggests that we do not want to give a note for the #define, but it seems a small price to pay for getting the other cases right.
(In reply to Tom Tromey from comment #16) > I've tripped across this enough that I've actually filed dups twice now. > > I think it would be best to change the ordering here. > That is, the initial error ought to generally be the > location of the outermost expansion. Then, the remaining > notes ought to delineate the macro expansions. > > While it is true that this will yield a sub-optimal result in some > cases, I think that it will have better results in the preponderance > of cases. That is, there's no way to be perfect here but gcc could be more > useful. I am starting to think the same here. In the unwinding, it seems less surprising to start from the code the user has the most chance to have written herself, that is, the code at the point of expansion of the outermost macro, rather than the point where the offending token was spelled -- which can be hidden anywhere. If everyone agrees, then I am okay to change the unwinding direction for the next stage 1. Manuel, Jonathan, what do you think?