| Message ID | 1455986783-7920-2-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/20/2016 09:46 AM, David Malcolm wrote: > Comment #18 of PR preprocessor/69126 reported a difficult-to-reproduce > re-occurrence of that bug, where attempts to suppress > -Wdeprecated-declarations > via a _Pragma could fail. > > The root cause is a bug in linemap_compare_locations when comparing > certain macro expansions with certain non-macro expansions. > > (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; it's erroneously > treating the _Pragma as if it were *after* the diagnostic (and hence > it doesn't affect it). > > But this is wrong: l0 is an ad-hoc loc, whereas l1 is a non-ad-hoc loc. > > It's clearly insane to do pure numeric comparisons of ad-hoc > locations with non-ad-hoc locations, since doing so will make the > latter always be "after" the former. The fix is simple: resolve > ad-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 the reporter of comment #18 of the PR 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, giving an ad-hoc > location for the _Pragma; in contrast, the diagnostic a single > character, and thus used a non-ad-hoc location. > > The attached patch fixes the issue, and adds test coverage, via a pair > of test cases c-c++-common/pr69126-2-{long|short}.c, where the "long" > version of the test case uses a macro name that's >=32 characters (thus > forcing the use of an ad-hoc location), which reproduced the issue. > > Given that this adds calls to get_location_from_adhoc_loc to the end > of linemap_compare_locations, I went ahead and removed the hand-inlined > copies from the top of the function (but I can avoid that change if > that's too much for stage 4). > > This also fixes the xfail in pr69543-1.c, as > "YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN" is longer than 31 chars and > was thus affected by this. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu in > combination with the previous patch. > > OK for trunk in stage 4? > > 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. OK. Thanks for tracking this down. jeff
diff --git a/gcc/testsuite/c-c++-common/pr69126-2-long.c b/gcc/testsuite/c-c++-common/pr69126-2-long.c new file mode 100644 index 0000000..f4f1964 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr69126-2-long.c @@ -0,0 +1,11 @@ +/* { dg-options "-Wdeprecated-declarations" } */ + +/* The macro's name is >= 32 characters long, and hence its location + requires an ad-hoc location. */ + +#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(); +} diff --git a/gcc/testsuite/c-c++-common/pr69126-2-short.c b/gcc/testsuite/c-c++-common/pr69126-2-short.c new file mode 100644 index 0000000..aee43e5 --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr69126-2-short.c @@ -0,0 +1,11 @@ +/* { dg-options "-Wdeprecated-declarations" } */ + +/* IGNORE_SHORT_MACRO is < 32 characters long, and hence its location + can be stored without needing an ad-hoc location. */ + +#define IGNORE_SHORT_MACRO _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") +__attribute__((deprecated)) void f(); +int main() { + IGNORE_SHORT_MACRO + f(); +} diff --git a/gcc/testsuite/c-c++-common/pr69543-1.c b/gcc/testsuite/c-c++-common/pr69543-1.c index bfb5270..bbf4759 100644 --- a/gcc/testsuite/c-c++-common/pr69543-1.c +++ b/gcc/testsuite/c-c++-common/pr69543-1.c @@ -3,8 +3,6 @@ /* Verify disabling a warning, where the _Pragma is within a macro, but the affected code is *not* in a macro. */ -/* TODO: XFAIL: why does g++ still emit a warning here? (works for C). */ - # define YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN \ _Pragma ("GCC diagnostic push") \ _Pragma ("GCC diagnostic ignored \"-Wuninitialized\"")\ @@ -16,6 +14,6 @@ void test (char yylval) { char *yyvsp; YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN - *++yyvsp = yylval; /* { dg-bogus "used uninitialized" "" { xfail { c++ } } } */ + *++yyvsp = yylval; YY_IGNORE_MAYBE_UNINITIALIZED_END } diff --git a/libcpp/line-map.c b/libcpp/line-map.c index e9175df..c05a001 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -1328,9 +1328,9 @@ linemap_compare_locations (struct line_maps *set, source_location l0 = pre, l1 = post; if (IS_ADHOC_LOC (l0)) - l0 = set->location_adhoc_data_map.data[l0 & MAX_SOURCE_LOCATION].locus; + l0 = get_location_from_adhoc_loc (set, l0); if (IS_ADHOC_LOC (l1)) - l1 = set->location_adhoc_data_map.data[l1 & MAX_SOURCE_LOCATION].locus; + l1 = get_location_from_adhoc_loc (set, l1); if (l0 == l1) return 0; @@ -1365,6 +1365,11 @@ linemap_compare_locations (struct line_maps *set, return i1 - i0; } + if (IS_ADHOC_LOC (l0)) + l0 = get_location_from_adhoc_loc (set, l0); + if (IS_ADHOC_LOC (l1)) + l1 = get_location_from_adhoc_loc (set, l1); + return l1 - l0; }
Comment #18 of PR preprocessor/69126 reported a difficult-to-reproduce re-occurrence of that bug, where attempts to suppress -Wdeprecated-declarations via a _Pragma could fail. The root cause is a bug in linemap_compare_locations when comparing certain macro expansions with certain non-macro expansions. (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; it's erroneously treating the _Pragma as if it were *after* the diagnostic (and hence it doesn't affect it). But this is wrong: l0 is an ad-hoc loc, whereas l1 is a non-ad-hoc loc. It's clearly insane to do pure numeric comparisons of ad-hoc locations with non-ad-hoc locations, since doing so will make the latter always be "after" the former. The fix is simple: resolve ad-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 the reporter of comment #18 of the PR 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, giving an ad-hoc location for the _Pragma; in contrast, the diagnostic a single character, and thus used a non-ad-hoc location. The attached patch fixes the issue, and adds test coverage, via a pair of test cases c-c++-common/pr69126-2-{long|short}.c, where the "long" version of the test case uses a macro name that's >=32 characters (thus forcing the use of an ad-hoc location), which reproduced the issue. Given that this adds calls to get_location_from_adhoc_loc to the end of linemap_compare_locations, I went ahead and removed the hand-inlined copies from the top of the function (but I can avoid that change if that's too much for stage 4). This also fixes the xfail in pr69543-1.c, as "YY_IGNORE_MAYBE_UNINITIALIZED_BEGIN" is longer than 31 chars and was thus affected by this. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu in combination with the previous patch. OK for trunk in stage 4? 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. --- gcc/testsuite/c-c++-common/pr69126-2-long.c | 11 +++++++++++ gcc/testsuite/c-c++-common/pr69126-2-short.c | 11 +++++++++++ gcc/testsuite/c-c++-common/pr69543-1.c | 4 +--- libcpp/line-map.c | 9 +++++++-- 4 files changed, 30 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/pr69126-2-long.c create mode 100644 gcc/testsuite/c-c++-common/pr69126-2-short.c