| Message ID | 1448301150-13289-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/23/2015 06:52 PM, David Malcolm wrote: > This patch fixes PR c/68473 by bulletproofing the new > diagnostic_show_locus implementation against ranges that finish before > they start (which can happen when using the C preprocessor), falling > back to simply printing a caret. Hmm, wouldn't it be better to avoid such a situation? Can you describe a bit more how exactly the macro expansion caused such a situation? Bernd
On Mon, 2015-11-23 at 18:59 +0100, Bernd Schmidt wrote: > On 11/23/2015 06:52 PM, David Malcolm wrote: > > This patch fixes PR c/68473 by bulletproofing the new > > diagnostic_show_locus implementation against ranges that finish before > > they start (which can happen when using the C preprocessor), falling > > back to simply printing a caret. > > Hmm, wouldn't it be better to avoid such a situation? Can you describe a > bit more how exactly the macro expansion caused such a situation? The issue is here: 1 /* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */ 2 3 extern long double fminl (long double __x, long double __y); 4 5 #define TEST_EQ(FUNC) do { \ 6 if ((long)FUNC##l(xl,xl) != (long)xl) \ 7 return; \ 8 } while (0) 9 10 void 11 foo (long double xl) 12 { 13 TEST_EQ (fmin); /* { dg-error "x87 register return with x87 disabled" } */ 14 } 16 /* { dg-begin-multiline-output "" } 17 TEST_EQ (fmin); 18 ^ 19 { dg-end-multiline-output "" } */ 20 21 /* { dg-begin-multiline-output "" } 22 if ((long)FUNC##l(xl,xl) != (long)xl) \ 23 ^~~~ 24 { dg-end-multiline-output "" } */ An error is emitted whilst expanding the macro at line 13, at input_location. This is at the expansion of this function call: fminl (xl, xl) Normally we'd emit a source range like this for a function call: fminl (xl, xl) ^~~~~~~~~~~~~~ However, once we fully resolve locations, the "fmin" part of "fminl" appears at line 13 here: 13 TEST_EQ (fmin); ^~~~ giving the location of the caret, and start of the range, whereas the rest of the the call is spelled here: 6 if ((long)FUNC##l(xl,xl) != (long)xl) \ ~~~~~~~ where the close paren gives the end of the range. It would be wrong to try to print the whole range (anything might be between lines 6 and 13). In theory we could attempt to try to handle this kind of thing by looking at the macro expansions, and to print something like: 13 TEST_EQ (fmin); ^~~~ 6 if ((long)FUNC##l(xl,xl) != (long)xl) \ ~~~~~~~~ or whatnot, but that strikes me as error-prone at this stage. The patch instead detects such a situation, and tries to handle things gracefully by falling back to simply printing a caret, without any underlines: pr68473-1.c: In function ‘foo’: pr68473-1.c:13:12: error: x87 register return with x87 disabled TEST_EQ (fmin); ^ pr68473-1.c:6:13: note: in definition of macro ‘TEST_EQ’ if ((long)FUNC##l(xl,xl) != (long)xl) \ ^~~~ Dave
On 11/23/2015 07:26 PM, David Malcolm wrote: > > In theory we could attempt to try to handle this kind of thing by > looking at the macro expansions, and to print something like: > > 13 TEST_EQ (fmin); > ^~~~ > 6 if ((long)FUNC##l(xl,xl) != (long)xl) \ > ~~~~~~~~ > > or whatnot, but that strikes me as error-prone at this stage. Could I ask you to spend some time looking at what would be involved at fixing it this way? In the end (assuming it doesn't prove to be a simple fix) we will probably go with your original patch for gcc-6, but it really goes against the grain to paper over a bug like this. Bernd
On 11/23/2015 10:52 AM, David Malcolm wrote: > This patch fixes PR c/68473 by bulletproofing the new > diagnostic_show_locus implementation against ranges that finish before > they start (which can happen when using the C preprocessor), falling > back to simply printing a caret. > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu; adds 7 new > PASS results to gcc.sum. I just ran into the same ICE with a different (simpler) example in bug 68839 and confirm that the patch fixes that one as well. Martin
On Tue, 2015-11-24 at 13:43 +0100, Bernd Schmidt wrote: > On 11/23/2015 07:26 PM, David Malcolm wrote: > > > > In theory we could attempt to try to handle this kind of thing by > > looking at the macro expansions, and to print something like: > > > > 13 TEST_EQ (fmin); > > ^~~~ > > 6 if ((long)FUNC##l(xl,xl) != (long)xl) \ > > ~~~~~~~~ > > > > or whatnot, but that strikes me as error-prone at this stage. > > Could I ask you to spend some time looking at what would be involved at > fixing it this way? In the end (assuming it doesn't prove to be a simple > fix) we will probably go with your original patch for gcc-6, but it > really goes against the grain to paper over a bug like this. I've been looking into this. The first issue we run into is that, currently, when we pass location_t values into rich_location, it expands them immediately and stores the result, which is thus throwing away any information on virtual macro locations. The first patch in the attached kit fixes that, storing location_t values (aka source_location) within rich_location, delaying their expansion until inside diagnostic_show_locus. Doing so also simplifies the code. In writing the above patch I noticed now muddled the terminology has become (range vs location), so the second patch in the kit rewords things; I believe it makes the code much clearer. The third patch in the kit is the earlier workaround for the bug; as before it degrades diagnostic-printing to just print a caret for the awkward cases, rather than attempting to print a range. This should mean that we fallback to the gcc 5 behavior for these cases. I've also added some new test cases based on duplicates filed against the bug; they show a variety of interesting patterns that would have to be handled if we were to use the 1st patch to fix things "properly". I've added some TODO comments to the testcases to describe my thoughts on how a proper fix might print them. I've successfully bootstrapped®rtested the combination of the patch kit on x86_64-pc-linux-gnu; adds 17 PASS results to gcc.sum. OK for trunk for gcc 6? (as 3 separate commits, assuming they individually bootstrap®rtest) Thanks Dave
On 12/11/2015 07:45 PM, David Malcolm wrote: > The third patch in the kit is the earlier workaround for the bug; as > before it degrades diagnostic-printing to just print a caret for the > awkward cases, rather than attempting to print a range. I'm a little confused now, do the first two patches actually help or are they just cleanups? If they have no immediate effect, let's postpone them to stage 1 and just do your initial workaround for now. Bernd
diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 9e51b95..968b3cb 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -444,7 +444,7 @@ layout::layout (diagnostic_context * context, { /* This diagnostic printer can only cope with "sufficiently sane" ranges. Ignore any ranges that are awkward to handle. */ - location_range *loc_range = richloc->get_range (idx); + const location_range *loc_range = richloc->get_range (idx); /* If any part of the range isn't in the same file as the primary location of this diagnostic, ignore the range. */ @@ -456,16 +456,38 @@ layout::layout (diagnostic_context * context, if (loc_range->m_caret.file != m_exploc.file) continue; + /* Everything is now known to be in the correct source file, + but it may require further sanitization. */ + layout_range ri (loc_range); + + /* If we have a range that finishes before it starts (perhaps + from something built via macro expansion), printing the + range is likely to be nonsensical. Also, attempting to do so + breaks assumptions within the printing code (PR c/68473). */ + if (loc_range->m_start.line > loc_range->m_finish.line) + { + /* Is this the primary location? */ + if (m_layout_ranges.length () == 0) + { + /* We want to print the caret for the primary location, but + we must sanitize away m_start and m_finish. */ + ri.m_start = ri.m_caret; + ri.m_finish = ri.m_caret; + } + else + /* This is a non-primary range; ignore it. */ + continue; + } + /* Passed all the tests; add the range to m_layout_ranges so that it will be printed. */ - layout_range ri (loc_range); m_layout_ranges.safe_push (ri); /* Update m_first_line/m_last_line if necessary. */ - if (loc_range->m_start.line < m_first_line) - m_first_line = loc_range->m_start.line; - if (loc_range->m_finish.line > m_last_line) - m_last_line = loc_range->m_finish.line; + if (ri.m_start.m_line < m_first_line) + m_first_line = ri.m_start.m_line; + if (ri.m_finish.m_line > m_last_line) + m_last_line = ri.m_finish.m_line; } /* Adjust m_x_offset. diff --git a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c index 0d8c7c5..3e38035 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c @@ -541,3 +541,20 @@ void test_quadratic (double a, double b, double c) { dg-end-multiline-output "" } */ } + +/* Reproducer for PR c/68473. */ + +extern long double fminl (long double __x, long double __y); +#define TEST_EQ(FUNC) FUNC##l(xl,xl) +void test_macro (long double xl) +{ + __emit_expression_range (0, TEST_EQ (fmin) ); /* { dg-warning "range" } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, TEST_EQ (fmin) ); + ^ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define TEST_EQ(FUNC) FUNC##l(xl,xl) + ^~~~ + { dg-end-multiline-output "" } */ +} diff --git a/gcc/testsuite/gcc.target/i386/pr68473-1.c b/gcc/testsuite/gcc.target/i386/pr68473-1.c new file mode 100644 index 0000000..ffffaa7 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr68473-1.c @@ -0,0 +1,24 @@ +/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */ + +extern long double fminl (long double __x, long double __y); + +#define TEST_EQ(FUNC) do { \ + if ((long)FUNC##l(xl,xl) != (long)xl) \ + return; \ + } while (0) + +void +foo (long double xl) +{ + TEST_EQ (fmin); /* { dg-error "x87 register return with x87 disabled" } */ +} + +/* { dg-begin-multiline-output "" } + TEST_EQ (fmin); + ^ + { dg-end-multiline-output "" } */ + +/* { dg-begin-multiline-output "" } + if ((long)FUNC##l(xl,xl) != (long)xl) \ + ^~~~ + { dg-end-multiline-output "" } */