$ cat /tmp/test.cpp #pragma GCC diagnostic push int f() { _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") int x; return 0; } #pragma GCC diagnostic pop #pragma GCC diagnostic push #define MACRO \ _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") \ int x; int g() { MACRO; return 0; } #pragma GCC diagnostic pop With GCC 4.8.5 or 5.3.1: $ gcc -O2 -Wall -c /tmp/test.cpp [nothing printed] With GCC 6: $ ~/gcc6/bin/gcc -O2 -Wall -c /tmp/test.cpp /tmp/test.cpp: In function ‘int g()’: /tmp/test.cpp:13:9: warning: unused variable ‘x’ [-Wunused-variable] int x; ^ /tmp/test.cpp:17:5: note: in expansion of macro ‘MACRO’ MACRO; ^~~~~ GCC 6 built from trunk last night (20160103).
Only affects C++, C is fine. Started with r226234. The C FE indeed still uses %q+D in this case.
Hi Jakub. I thought it was simply matter of using location_of instead of DECL_SOURCE_LOCATION as I did in other places in that patch set, but in fact it doesn't work, in order to avoid the spurious warning a warning and '+' instead of a locally computed location + warning_at seems really necessary. Certainly I can revert the specific change of mine (would be line #633 and #634 in the current decl.c) and align the C++ FE with the C FE, that fixes the regression and I'm pretty sure would not introduce other wrt the status pre-r226234, but I don't fully understand why that is necessary, there are interactions with the handling of macros which I don't fully understand, at the moment. Let me know if you want me to do that for now.
I don't understand those interactions either, and agree it would be nice to first understand those. Which is why I've CCed Dodji and David, in hopes they could shed some light into this. For whatever reason, the DECL_SOURCE_LOCATION of the x decl when it comes from macro is the locus where the macro is used rather than the locus in the macro.
(In reply to Jakub Jelinek from comment #3) > I don't understand those interactions either, and agree it would be nice to > first understand those. Which is why I've CCed Dodji and David, in hopes > they could shed some light into this. FWIW I'm poking at it now. > For whatever reason, the DECL_SOURCE_LOCATION of the x decl when it comes > from macro is the locus where the macro is used rather than the locus in the > macro. I would have expected the DECL_SOURCE_LOCATION to be the locus of where the macro is used, and indeed that was the case with gcc 4.8.3: (gdb) p /x decl->decl_minimal.locus $11 = 0x7ffffffd (which is a macro expansion location) (gdb) call inform (decl->decl_minimal.locus, "DECL_SOURCE_LOCATION of 2nd x with gcc 4.8.3") /tmp/test.cc:13:9: note: DECL_SOURCE_LOCATION of 2nd x with gcc 4.8.3 int x; ^ /tmp/test.cc:17:5: note: in expansion of macro ‘MACRO’ MACRO; ^
As far as I can tell: * _Pragma is handled by _cpp_do__Pragma which injects pragma tokens into the token stream * the resulting "ignore" pragmas are handled by gcc/c-family/c-pragma.c:handle_pragma_diagnostic which calls: * control_warning_option, which calls * diagnostic_classify_diagnostic, which leads to an element being pushed onto context->classification_history with location as the location of the token. When handling the warning/warning_at, diagnostic_report_diagnostic has this logic: 695 /* This tests for #pragma diagnostic changes. */ 696 if (context->n_classification_history > 0) 697 { 698 int i; 699 /* FIXME: Stupid search. Optimize later. */ 700 for (i = context->n_classification_history - 1; i >= 0; i --) 701 { 702 if (linemap_location_before_p 703 (line_table, 704 context->classification_history[i].location, 705 location)) Note the call to linemap_location_before_p, which calls linemap_compare_locations. That said, the location of the token from _Pragma looks wrong; note the underlines here: Breakpoint 2, handle_pragma_diagnostic (dummy=<optimized out>) at ../../src/gcc/c-family/c-pragma.c:760 760 kind = DK_IGNORED; (gdb) call inform (loc, "1st ignore") /tmp/test.cc: In function ‘int f()’: /tmp/test.cc:4:16: note: 1st ignore _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") ^~~~~~~ (gdb) cont Continuing. Breakpoint 2, handle_pragma_diagnostic (dummy=<optimized out>) at ../../src/gcc/c-family/c-pragma.c:760 760 kind = DK_IGNORED; (gdb) call inform (loc, "2nd ignore") /tmp/test.cc: In function ‘int g()’: /tmp/test.cc:17:16: note: 2nd ignore MACRO; ^
(In reply to David Malcolm from comment #5) [...snip...] > That said, the location of the token from _Pragma looks wrong; note the > underlines here: > > Breakpoint 2, handle_pragma_diagnostic (dummy=<optimized out>) at > ../../src/gcc/c-family/c-pragma.c:760 > 760 kind = DK_IGNORED; > (gdb) call inform (loc, "1st ignore") > /tmp/test.cc: In function ‘int f()’: > /tmp/test.cc:4:16: note: 1st ignore > _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") > ^~~~~~~ > (gdb) cont > Continuing. > > Breakpoint 2, handle_pragma_diagnostic (dummy=<optimized out>) at > ../../src/gcc/c-family/c-pragma.c:760 > 760 kind = DK_IGNORED; > (gdb) call inform (loc, "2nd ignore") > /tmp/test.cc: In function ‘int g()’: > /tmp/test.cc:17:16: note: 2nd ignore > MACRO; > ^ I got this slightly wrong, the locations passed to control_warning_option (and thus put in the classification_history) are these: (gdb) p loc $1 = 173682 (gdb) call inform (loc, "1st ignore") /tmp/test.cc: In function ‘int f()’: /tmp/test.cc:4:24: note: 1st ignore _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") ^~~~~~~~~~~~~~~~~~~ (gdb) p loc $2 = 226930 (gdb) call inform (loc, "2nd ignore") /tmp/test.cc: In function ‘int g()’: /tmp/test.cc:17:24: note: 2nd ignore MACRO; ^ (the above is trunk). That said gcc 4.8.3 appears to generate similarly strange locations for these tokens: (gdb) p loc $15 = 5428 (gdb) call inform (loc, "1st ignore") /tmp/test.cc:4:24: note: 1st ignore _Pragma("GCC diagnostic ignored \"-Wunused-variable\"") ^ (gdb) p loc $16 = 7092 (gdb) call inform (loc, "2nd ignore") /tmp/test.cc: In function ‘int g()’: /tmp/test.cc:17:24: note: 2nd ignore MACRO; ^
Thanks David. For the record, the current status of the C++ FE is that of *zero* uses of +D (or q+D) in warnings, consistently with Manuel's advice a while ago which I tried to implement. Thus, IMHO, we need a pretty robust reason to reintroduce one, outside a temporary regression fix. Looks like the problem is indeed elsewhere...
Looking at calls to linemap_compare_locations, the crucial comparison is the comparison of the location of the 2nd diagnostic with that of the "ignore" pragma: Breakpoint 2, linemap_compare_locations (set=0x7ffff7ffb000, pre=226930, post=post@entry=2147483645) at ../../src/libcpp/line-map.c:1311 (gdb) call inform (226930, "pre, the location of the ignore") /tmp/test.cc:17:24: note: pre, the location of the ignore MACRO; ^ (gdb) call inform (2147483645, "post, the location of the diagnostic") /tmp/test.cc:13:9: note: post, the location of the diagnostic int x; ^ /tmp/test.cc:17:5: note: in expansion of macro ‘MACRO’ MACRO; ^~~~~ The initial value of l1 == post (location of the diagnostic): (gdb) p /x l1 $15 = 0x7ffffffd linemap_compare_locations expands macro locations at LRK_MACRO_EXPANSION_POINT and obtains: (gdb) p l1 $19 = 226308 (gdb) call inform (226308, "") /tmp/test.cc:17:5: note: MACRO; ^~~~~ and hence treats the location of "post" (the diagnostic) as the "M" of the macro expansion point. However, the location of the "ignore" within the macro definition is 226930, which is *after* that "M": (gdb) call inform (226930, "pre, the location of the ignore") /tmp/test.cc:17:24: note: pre, the location of the ignore MACRO; ^ Hence that strange-looking synthesized location of the "ignore" directive within the tokens synthesized by _cpp_do__Pragma is effectively *after* that of the macro expansion point. Hence the "ignore" is treated as occurring after the diagnostic, hence the "ignore" doesn't affect the diagnostic, and the warning is emitted. FWIW, there's also some logic in linemap_compare_locations for handling locations in the same macro expansion, but this isn't the case here as the location of the pragma has been recorded as an ordinary location.
Running with gcc-4.8.3, it appears that the strange-looking location of the "ignore" is pre-existing behavior, and that the change in r226234 of where the diagnostic occurs has simply exposed it. Breakpoint 18, linemap_compare_locations (set=0x7ffff7ffb000, pre=7092, post=post@entry=7209) at ../../libcpp/line-map.c:1023 (gdb) call inform (7092, "pre, the location of the ignore") /tmp/test.cc: In function ‘int g()’: /tmp/test.cc:17:24: note: pre, the location of the ignore MACRO; ^ (gdb) call inform (7209, "post, the location of the diagnostic") /tmp/test.cc:18:13: note: post, the location of the diagnostic return 0; ^
Hence I *think* the issue here is that the locations of the tokens within libcpp/directives.c:destringize_and_run don't respect macro expansions, leading to the strange location for the ignore directive that is slightly after the relevant macro expansion. However, that function is full of comments like /* Ugh; an awful kludge. We are really not set up to be lexing and: /* ??? Antique Disgusting Hack. What does this do? */ and the like, so am not sure I want to touch it... Perhaps this could be papered over by reverting r226234, but that would regress the removal of %q+D mentioned in comment #7, and there are presumably other ways to combine _Pragma and macros that would show up issues this way (though these behaviors are presumably already present in earlier gcc releases).
Perhaps it would be possible to arrange for the lexing of _Pragma string to get the right locations for the simple case where _Pragma argument is a single string literal without escapes etc., but as soon as escapes are there, or say stringified arguments are added, and string literals concatenated, it is going to be really difficult. But if we at least could let the easiest cases work fine, it would be really nice.
Patch posted here: https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01212.html
(In reply to Jakub Jelinek from comment #3) > For whatever reason, the DECL_SOURCE_LOCATION of the x decl when it comes > from macro is the locus where the macro is used rather than the locus in the > macro. The actual issue is that "GCC diagnostic" uses the location passed to the diagnostic function explicitly (or input_location), never the location set by '+'. That is, warning("%q+D") uses input_location for testing against the pragma but DECL_SOURCE_LOCATION for reporting the warning. Of course, such behaviour breaks other cases, which is one reason to avoid '+'.
Related bug 52116
(In reply to Paolo Carlini from comment #2) > the regression and I'm pretty sure would not introduce other wrt the status > pre-r226234, but I don't fully understand why that is necessary, there are Wasn't the reason for removing '+' a bug report showing a similar case (#pragma diagnostic being ignored because '+' does not affect the pragma)? Cases that did work before but don't work now just worked by chance. Unrelated changes to input_location would also break them. David's fix is a step in the right direction.
Author: dmalcolm Date: Wed Jan 27 18:57:51 2016 New Revision: 232893 URL: https://gcc.gnu.org/viewcvs?rev=232893&root=gcc&view=rev Log: libcpp: use better locations for _Pragma tokens (preprocessor/69126) gcc/testsuite/ChangeLog: PR preprocessor/69126 * c-c++-common/pr69126.c: New test case. libcpp/ChangeLog: PR preprocessor/69126 * directives.c (destringize_and_run): Add expansion_loc param; use it when handling unexpanded pragmas to fixup the locations of the synthesized tokens. (_cpp_do__Pragma): Add expansion_loc param and use it when calling destringize_and_run. * internal.h (_cpp_do__Pragma): Add expansion_loc param. * macro.c (builtin_macro): Pass expansion location of _Pragma to _cpp_do__Pragma. Added: trunk/gcc/testsuite/c-c++-common/pr69126.c Modified: trunk/gcc/testsuite/ChangeLog trunk/libcpp/ChangeLog trunk/libcpp/directives.c trunk/libcpp/internal.h trunk/libcpp/macro.c
Should be fixed as of r232893; marking this as resolved.
A GCC trunk build past r232893 started causing building LibreOffice to fail now for me, with a _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") coming from a macro expansion being ignored, thus causing a -Werror=deprecated-declarations. I cannot easily reduce the test case further than what I have below. I tried to expand the remaining includes inline in the main source file, but then ran into strange effects where e.g. removing an arbitrary single line from the middle of a multi-line /*...*/ comment made the problem go away, while replacing the line with an empty line made the problem stay. The first #include is for a file coming from LO's own bundled Boost, the remaining #includes are from the (F22, x86_64) system. (Also, the problem only happens with -O or higher.) > $ cat test.cc > #include <boost/property_tree/json_parser.hpp> > #define __GDK_H_INSIDE__ > #include <gdk/gdkversionmacros.h> > #include <gdk/gdkscreen.h> > #include <gdk/gdkcairo.h> > #include <gdk/gdkcursor.h> > > #define IGNORE _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") > __attribute__((deprecated)) void f(); > int main() { > IGNORE > f(); > } > $ /home/sbergman/gcc/trunk/inst/bin/g++ -Werror -O -c test.cc \ > -I/home/sbergman/lo/core/workdir/UnpackedTarball/boost \ > -I/usr/include/gtk-3.0 -I/usr/include/cairo -I/usr/include/pango-1.0 \ > -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/glib-2.0 \ > -I/usr/lib64/glib-2.0/include > > test.cc: In function ‘int main()’: > test.cc:12:5: error: ‘void f()’ is deprecated [-Werror=deprecated-declarations] > f(); > ^ > test.cc:9:34: note: declared here > __attribute__((deprecated)) void f(); > ^ > test.cc:12:7: error: ‘void f()’ is deprecated [-Werror=deprecated-declarations] > f(); > ^ > test.cc:9:34: note: declared here > __attribute__((deprecated)) void f(); > ^ > cc1plus: all warnings being treated as errors
Reopening then.
Does the http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02347.html workaround help here in any way?
In any case, we need much better testsuite coverage for the cases that the various projects use (glib2, LO, ...), so that we don't regress on those without knowing about it months later.
(In reply to Jakub Jelinek from comment #20) > Does the http://gcc.gnu.org/ml/gcc-patches/2016-01/msg02347.html workaround > help here in any way? No, unfortunately not. Doesn't change the behaviour in any way.
(In reply to Stephan Bergmann from comment #18) > (Also, the problem > only happens with -O or higher.) That is weird. If you use "GCC diagnostic warning" instead of "ignored", you should be able to see some changes in locations between -O0 and -O1 and before/after the bug appeared, don't you?
(In reply to Manuel López-Ibáñez from comment #23) > That is weird. If you use "GCC diagnostic warning" instead of "ignored", you > should be able to see some changes in locations between -O0 and -O1 and > before/after the bug appeared, don't you? No. The reported locations remain unaffected (errors/warnings at test.cc:12:5 and test.cc:12:7, notes at test.cc:9:34). Just that (after replacing "ignored" with "warning") they get reported as errors with -O1 and r232893 included, and as warnings otherwise (i.e., either with -O0 and r232893 included, or with either -O0/1 and r232893 reverted).
Stephan provided me with a tarball of his reproducer (thanks!), and I'm able to reproduce this locally. The root cause is a bug in linemap_compare_locations. (gdb) call inform (pre, "pre") test.cc:8:16: note: pre #define IGNORE _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") ^ test.cc:8:16: note: in definition of macro ‘IGNORE’ #define IGNORE _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") ^~~~~~~ (gdb) call inform (post, "post") test.cc:12:5: note: post f(); ^ After macro expansion, we have (at the end of linemap_compare_locations): (gdb) p /x l0 $13 = 0x800101ec (gdb) p /x l1 $14 = 0x50684c05 and hence: (gdb) p /x l1 - l0 $23 = 0xd0674a19 it's effectively negative, and so before_p is false. But this is wrong: l0 is an ad-hoc loc, whereas l1 is a non-ad-hoc loc. It's clearly insane to treat all ad-hoc locations as being later than all non-ad-hoc locations. The fix is simple: resolve ads-hoc locations at the end of linemap_compare_locations. For this bug to occur, we need a location for the macro name that's an ad-hoc location, and a location for the diagnostic that's *not* an ad-hoc location. The reason it triggered for Stephan is that the sheer quantity of code in his reproducer meant that both locations in question were above LINE_MAP_MAX_LOCATION_WITH_PACKED_RANGES (but below LINE_MAP_MAX_LOCATION_WITH_COLS), so range-packing was disabled, in particular for the "IGNORE" macro. I can reproduce it trivially by using a macro name that's >=32 characters (thus forcing the use of an ad-hoc location): /* { dg-options "-Wdeprecated-declarations" } */ #define IGNORE_WHERE_MACRO_IS_LONGER_THAN_31_CHARS _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") __attribute__((deprecated)) void f(); int main() { IGNORE_WHERE_MACRO_IS_LONGER_THAN_31_CHARS f(); } I have a patch that seems to work for this test case; am testing it more thoroughly now.
(In reply to David Malcolm from comment #25) [...] > I have a patch that seems to work for this test case; am testing it more > thoroughly now. Candidate patch posted here: https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01424.html (see also: https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01425.html )
Many thanks once more, David!
Author: dmalcolm Date: Tue Feb 23 17:39:16 2016 New Revision: 233637 URL: https://gcc.gnu.org/viewcvs?rev=233637&root=gcc&view=rev Log: Add test coverage for _Pragma (PR preprocessor 69126, 69543, 69558) We had some regressions in the ability for _Pragma to disable a warning (PR preprocessor/69126, PR preprocessor/69543, PR preprocessor/69558). This patch attempts to add more test coverage for this, for the various combinations of: - various warnings: -Wunused-variable -Wuninitialized -Wdeprecated-declarations - various combinations of location of _Pragma relative to location of the warning: - _Pragma is in a macro, warning isn't a macro - neither is in a macro - _Pragma isnt't in a macro, warning is in a macro - in different macros - both in the same macro - C vs C++ frontend. It adds some XFAILs: - pr69543-1.c for C++ (fixed in the followup patch) - pr69543-3.c for both C and C++ - pr69543-4.c for both C and C++ - pr69558.c for C++ (moving it from gcc.dg to c-c++-common, marking it as xfail for C++ for now) gcc/testsuite/ChangeLog: PR preprocessor/69126 PR preprocessor/69543 PR preprocessor/69558 * c-c++-common/pr69126.c (MACRO_1, test_1): New. (f): Rename to... (test_2): ...this, and add leading comment. (MACRO_3, test_3): New. (MACRO_4A, MACRO_4B, test_4): New. (MACRO): Rename to... (MACRO_5): ...this. (g): Rename to... (test_5): ...this, updating for renaming of MACRO, and add leading comment. * c-c++-common/pr69543-1.c: New. * c-c++-common/pr69543-2.c: New. * c-c++-common/pr69543-3.c: New. * c-c++-common/pr69543-4.c: New. * c-c++-common/pr69558-1.c: New. * c-c++-common/pr69558-2.c: New. * c-c++-common/pr69558-3.c: New. * c-c++-common/pr69558-4.c: New. * gcc.dg/pr69558.c: Move to... * c-c++-common/pr69558.c: ...here. Add dg-bogus directives, with xfail for c++. Added: trunk/gcc/testsuite/c-c++-common/pr69543-1.c trunk/gcc/testsuite/c-c++-common/pr69543-2.c trunk/gcc/testsuite/c-c++-common/pr69543-3.c trunk/gcc/testsuite/c-c++-common/pr69543-4.c trunk/gcc/testsuite/c-c++-common/pr69558-1.c trunk/gcc/testsuite/c-c++-common/pr69558-2.c trunk/gcc/testsuite/c-c++-common/pr69558-3.c trunk/gcc/testsuite/c-c++-common/pr69558-4.c trunk/gcc/testsuite/c-c++-common/pr69558.c - copied, changed from r233636, trunk/gcc/testsuite/gcc.dg/pr69558.c Removed: trunk/gcc/testsuite/gcc.dg/pr69558.c Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/c-c++-common/pr69126.c
Author: dmalcolm Date: Tue Feb 23 17:44:28 2016 New Revision: 233638 URL: https://gcc.gnu.org/viewcvs?rev=233638&root=gcc&view=rev Log: PR preprocessor/69126: avoid comparing ad-hoc and non-ad-hoc locations gcc/testsuite/ChangeLog: PR preprocessor/69126 PR preprocessor/69543 * c-c++-common/pr69126-2-long.c: New test. * c-c++-common/pr69126-2-short.c: New test. * c-c++-common/pr69543-1.c: Remove xfail. libcpp/ChangeLog: PR preprocessor/69126 PR preprocessor/69543 * line-map.c (linemap_compare_locations): At the function top, replace inlined bodies of get_location_from_adhoc_loc with calls to get_location_from_adhoc_loc. Add a pair of calls to get_location_from_adhoc_loc at the bottom of the function, to avoid meaningless comparisons of ad-hoc and non-ad-hoc locations. Added: trunk/gcc/testsuite/c-c++-common/pr69126-2-long.c trunk/gcc/testsuite/c-c++-common/pr69126-2-short.c Modified: trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/c-c++-common/pr69543-1.c trunk/libcpp/ChangeLog trunk/libcpp/line-map.c
The issue reported in comment #18 should be fixed as of r233638; marking this one as resolved again.