On Mon, 2017-07-03 at 10:07 -0600, Jeff Law wrote: > On 02/02/2017 01:53 PM, David Malcolm wrote: > > PR c++/79300 identifies an issue in which diagnostics_show_locus > > prints the wrong end-point for a range within a macro: > > > > assert ((p + val_size) - buf == encoded_len); > > ~~~~~~~~~~~~~~~~~~~~~^~~~ > > > > as opposed to: > > > > assert ((p + val_size) - buf == encoded_len); > > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~ > > > > The caret, start and finish locations of this compound location are > > all virtual locations. > > > > The root cause is that when diagnostic-show-locus.c's layout ctor > > expands the caret and end-points, it calls > > linemap_client_expand_location_to_spelling_point > > which (via expand_location_1) unwinds the macro expansions, and > > then calls linemap_expand_location. Doing so implicitly picks the > > *caret* location for any virtual locations, and so in the above > > case > > it picks these spelling locations for the three parts of the > > location: > > > > assert ((p + val_size) - buf == encoded_len); > > ^ ^ ^ > > START | FINISH > > CARET > > > > and so erroneously strips the underlining from the final token, > > apart > > from its first character. > > > > The fix is for layout's ctor to indicate that it wants the > > start/finish > > locations in such a situation, adding a new param to > > linemap_client_expand_location_to_spelling_point, so that > > expand_location_1 can handle this case by extracting the relevant > > part > > of the unwound compound location, and thus choose: > > > > assert ((p + val_size) - buf == encoded_len); > > ^ ^ ^ > > START | FINISH > > CARET > > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > > > OK for stage 4, or should I wait until stage 1? > > > > gcc/ChangeLog: > > PR c++/79300 > > * diagnostic-show-locus.c (layout::layout): Use start and > > finish > > spelling location for the start and finish of each range. > > * genmatch.c > > (linemap_client_expand_location_to_spelling_point): > > Add unused aspect param. > > * input.c (expand_location_1): Add "aspect" param, and use it > > to access the correct part of the location. > > (expand_location): Pass LOCATION_ASPECT_CARET to new param of > > expand_location_1. > > (expand_location_to_spelling_point): Likewise. > > (linemap_client_expand_location_to_spelling_point): Add > > "aspect" > > param, and pass it to expand_location_1. > > > > gcc/testsuite/ChangeLog: > > PR c++/79300 > > * c-c++-common/Wmisleading-indentation-3.c (fn_14): Update > > expected underlining within macro expansion. > > * c-c++-common/pr70264.c: Likewise. > > * g++.dg/plugin/diagnostic-test-expressions-1.C > > (test_within_macro_1): New test. > > (test_within_macro_2): Likewise. > > (test_within_macro_3): Likewise. > > (test_within_macro_4): Likewise. > > * gcc.dg/format/diagnostic-ranges.c (test_macro_3): Update > > expected underlining within macro expansion. > > (test_macro_4): Likewise. > > * gcc.dg/plugin/diagnostic-test-expressions-1.c > > (test_within_macro_1): New test. > > (test_within_macro_2): Likewise. > > (test_within_macro_3): Likewise. > > (test_within_macro_4): Likewise. > > * gcc.dg/spellcheck-fields-2.c (test_macro): Update expected > > underlining within macro expansion. > > > > libcpp/ChangeLog: > > PR c++/79300 > > * include/line-map.h (enum location_aspect): New enum. > > (linemap_client_expand_location_to_spelling_point): Add > > enum location_aspect param. > > * line-map.c (source_range::intersects_line_p): Update for new > > param of linemap_client_expand_location_to_spelling_point. > > (rich_location::get_expanded_location): Likewise. > > (fixit_insert::affects_line_p): Likewise. > So we punted this to gcc-8 stage1. Now that I've finally looked at > it, > it looks good to me. > > Sorry for the long wait.
Thanks; looks like I forgot to apply this one when stage 1 reopened. The libcpp part of the patch needed a bit of reworking due to changes I've made to the internals of fix-it hints. For reference, here's what I committed to trunk (as r250022), after bootstrap®rtest on x86_64-pc-linux-gnu: gcc/ChangeLog: PR c++/79300 * diagnostic-show-locus.c (layout::layout): Use start and finish spelling location for the start and finish of each range. * genmatch.c (linemap_client_expand_location_to_spelling_point): Add unused aspect param. * input.c (expand_location_1): Add "aspect" param, and use it to access the correct part of the location. (expand_location): Pass LOCATION_ASPECT_CARET to new param of expand_location_1. (expand_location_to_spelling_point): Likewise. (linemap_client_expand_location_to_spelling_point): Add "aspect" param, and pass it to expand_location_1. gcc/testsuite/ChangeLog: PR c++/79300 * c-c++-common/Wmisleading-indentation-3.c (fn_14): Update expected underlining within macro expansion. * c-c++-common/pr70264.c: Likewise. * g++.dg/plugin/diagnostic-test-expressions-1.C (test_within_macro_1): New test. (test_within_macro_2): Likewise. (test_within_macro_3): Likewise. (test_within_macro_4): Likewise. * gcc.dg/format/diagnostic-ranges.c (test_macro_3): Update expected underlining within macro expansion. (test_macro_4): Likewise. * gcc.dg/plugin/diagnostic-test-expressions-1.c (test_within_macro_1): New test. (test_within_macro_2): Likewise. (test_within_macro_3): Likewise. (test_within_macro_4): Likewise. * gcc.dg/spellcheck-fields-2.c (test_macro): Update expected underlining within macro expansion. libcpp/ChangeLog: PR c++/79300 * include/line-map.h (enum location_aspect): New enum. (linemap_client_expand_location_to_spelling_point): Add enum location_aspect param. * line-map.c (rich_location::get_expanded_location): Update for new param of linemap_client_expand_location_to_spelling_point. (rich_location::maybe_add_fixit): Likewise. (fixit_hint::affects_line_p): Likewise. --- gcc/diagnostic-show-locus.c | 9 ++- gcc/genmatch.c | 3 +- gcc/input.c | 52 +++++++++++--- .../c-c++-common/Wmisleading-indentation-3.c | 2 +- gcc/testsuite/c-c++-common/pr70264.c | 2 +- .../g++.dg/plugin/diagnostic-test-expressions-1.C | 78 +++++++++++++++++++++ gcc/testsuite/gcc.dg/format/diagnostic-ranges.c | 4 +- .../gcc.dg/plugin/diagnostic-test-expressions-1.c | 79 ++++++++++++++++++++++ gcc/testsuite/gcc.dg/spellcheck-fields-2.c | 2 +- libcpp/include/line-map.h | 12 +++- libcpp/line-map.c | 18 +++-- 11 files changed, 236 insertions(+), 25 deletions(-) diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 8bf4d9e..8a4fd5f 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -788,11 +788,14 @@ layout::layout (diagnostic_context * context, /* Expand the various locations. */ expanded_location start - = linemap_client_expand_location_to_spelling_point (src_range.m_start); + = linemap_client_expand_location_to_spelling_point + (src_range.m_start, LOCATION_ASPECT_START); expanded_location finish - = linemap_client_expand_location_to_spelling_point (src_range.m_finish); + = linemap_client_expand_location_to_spelling_point + (src_range.m_finish, LOCATION_ASPECT_FINISH); expanded_location caret - = linemap_client_expand_location_to_spelling_point (loc_range->m_loc); + = linemap_client_expand_location_to_spelling_point + (loc_range->m_loc, LOCATION_ASPECT_CARET); /* If any part of the range isn't in the same file as the primary location of this diagnostic, ignore the range. */ diff --git a/gcc/genmatch.c b/gcc/genmatch.c index f20e39f..7045bb9 100644 --- a/gcc/genmatch.c +++ b/gcc/genmatch.c @@ -61,7 +61,8 @@ static struct line_maps *line_table; This is the implementation for genmatch. */ expanded_location -linemap_client_expand_location_to_spelling_point (source_location loc) +linemap_client_expand_location_to_spelling_point (source_location loc, + enum location_aspect) { const struct line_map_ordinary *map; loc = linemap_resolve_location (line_table, loc, LRK_SPELLING_LOCATION, &map); diff --git a/gcc/input.c b/gcc/input.c index 8071810..0480eb2 100644 --- a/gcc/input.c +++ b/gcc/input.c @@ -147,11 +147,14 @@ static const size_t fcache_line_record_size = 100; associated line/column) in the context of a macro expansion, the returned location is the first one (while unwinding the macro location towards its expansion point) that is in real source - code. */ + code. + + ASPECT controls which part of the location to use. */ static expanded_location expand_location_1 (source_location loc, - bool expansion_point_p) + bool expansion_point_p, + enum location_aspect aspect) { expanded_location xloc; const line_map_ordinary *map; @@ -181,8 +184,36 @@ expand_location_1 (source_location loc, loc, NULL); lrk = LRK_SPELLING_LOCATION; } - loc = linemap_resolve_location (line_table, loc, - lrk, &map); + loc = linemap_resolve_location (line_table, loc, lrk, &map); + + /* loc is now either in an ordinary map, or is a reserved location. + If it is a compound location, the caret is in a spelling location, + but the start/finish might still be a virtual location. + Depending of what the caller asked for, we may need to recurse + one level in order to resolve any virtual locations in the + end-points. */ + switch (aspect) + { + default: + gcc_unreachable (); + /* Fall through. */ + case LOCATION_ASPECT_CARET: + break; + case LOCATION_ASPECT_START: + { + source_location start = get_start (loc); + if (start != loc) + return expand_location_1 (start, expansion_point_p, aspect); + } + break; + case LOCATION_ASPECT_FINISH: + { + source_location finish = get_finish (loc); + if (finish != loc) + return expand_location_1 (finish, expansion_point_p, aspect); + } + break; + } xloc = linemap_expand_location (line_table, map, loc); } @@ -773,7 +804,8 @@ is_location_from_builtin_token (source_location loc) expanded_location expand_location (source_location loc) { - return expand_location_1 (loc, /*expansion_point_p=*/true); + return expand_location_1 (loc, /*expansion_point_p=*/true, + LOCATION_ASPECT_CARET); } /* Expand the source location LOC into a human readable location. If @@ -785,7 +817,8 @@ expand_location (source_location loc) expanded_location expand_location_to_spelling_point (source_location loc) { - return expand_location_1 (loc, /*expansion_point_p=*/false); + return expand_location_1 (loc, /*expansion_point_p=*/false, + LOCATION_ASPECT_CARET); } /* The rich_location class within libcpp requires a way to expand @@ -795,12 +828,13 @@ expand_location_to_spelling_point (source_location loc) to do this. This is the implementation for libcommon.a (all host binaries), - which simply calls into expand_location_to_spelling_point. */ + which simply calls into expand_location_1. */ expanded_location -linemap_client_expand_location_to_spelling_point (source_location loc) +linemap_client_expand_location_to_spelling_point (source_location loc, + enum location_aspect aspect) { - return expand_location_to_spelling_point (loc); + return expand_location_1 (loc, /*expansion_point_p=*/false, aspect); } diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c index 6482b00..870ba72 100644 --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c @@ -68,7 +68,7 @@ void fn_14 (void) /* { dg-begin-multiline-output "" } for ((VAR) = (START); (VAR) < (STOP); (VAR++)) - ^ + ^~~ { dg-end-multiline-output "" } */ /* { dg-begin-multiline-output "" } FOR_EACH (i, 0, 10) diff --git a/gcc/testsuite/c-c++-common/pr70264.c b/gcc/testsuite/c-c++-common/pr70264.c index 815aad1..c446942 100644 --- a/gcc/testsuite/c-c++-common/pr70264.c +++ b/gcc/testsuite/c-c++-common/pr70264.c @@ -5,7 +5,7 @@ X /* { dg-begin-multiline-output "" } #define X __LINE__ - ^ + ^~~~~~~~ { dg-end-multiline-output "" } */ /* { dg-begin-multiline-output "" } X diff --git a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C index 2c004f3..12f205d 100644 --- a/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C +++ b/gcc/testsuite/g++.dg/plugin/diagnostic-test-expressions-1.C @@ -848,3 +848,81 @@ tests::test_method_calls () ~~~~~~~~~~~~~~~~~~^~ { dg-end-multiline-output "" } */ } + +/* Various tests of locations involving macros. */ + +void test_within_macro_1 (int lhs, int rhs) +{ +#define MACRO_1(EXPR) EXPR + + __emit_expression_range (0, MACRO_1 (lhs == rhs)); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_1 (lhs == rhs)); + ~~~~^~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_1(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_1 +} + +void test_within_macro_2 (int lhs, int rhs) +{ +#define MACRO_2(EXPR) EXPR + + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + ~~~~^~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + ^~~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_2(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_2 +} + +void test_within_macro_3 (int lhs, int rhs) +{ +#define MACRO_3(EXPR) EXPR + + __emit_expression_range (0, MACRO_3 (lhs) == MACRO_3 (rhs)); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_3 (lhs) == MACRO_3 (rhs)); + ^ + { dg-end-multiline-output "" } */ + +#undef MACRO_3 +} + +void test_within_macro_4 (int lhs, int rhs) +{ +#define MACRO_4(EXPR) EXPR + + __emit_expression_range (0, MACRO_4 (MACRO_4 (lhs) == MACRO_4 (rhs))); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_4 (MACRO_4 (lhs) == MACRO_4 (rhs))); + ^ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_4(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_4 +} diff --git a/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c index cab30f2..8cce5b3 100644 --- a/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c +++ b/gcc/testsuite/gcc.dg/format/diagnostic-ranges.c @@ -278,7 +278,7 @@ void test_macro_3 (const char *msg) printf(FMT_STRING, msg); /* { dg-message "10: in expansion of macro 'FMT_STRING" } */ /* { dg-begin-multiline-output "" } #define FMT_STRING "hello %i world" - ^ + ^~~~~~~~~~~~~~~~ { dg-end-multiline-output "" } */ /* { dg-begin-multiline-output "" } printf(FMT_STRING, msg); @@ -293,7 +293,7 @@ void test_macro_4 (const char *msg) printf(FMT_STRING "\n", msg); /* { dg-message "10: in expansion of macro 'FMT_STRING" } */ /* { dg-begin-multiline-output "" } #define FMT_STRING "hello %i world" - ^ + ^~~~~~~~~~~~~~~~ { dg-end-multiline-output "" } */ /* { dg-begin-multiline-output "" } printf(FMT_STRING "\n", msg); 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 b0dbc05..03b7804 100644 --- a/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c +++ b/gcc/testsuite/gcc.dg/plugin/diagnostic-test-expressions-1.c @@ -708,3 +708,82 @@ baz"); ~~~~ { dg-end-multiline-output "" } */ } + +/* Various tests of locations involving macros. */ + +void test_within_macro_1 (int lhs, int rhs) +{ +#define MACRO_1(EXPR) EXPR + + __emit_expression_range (0, MACRO_1 (lhs == rhs)); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_1 (lhs == rhs)); + ~~~~^~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_1(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_1 +} + +void test_within_macro_2 (int lhs, int rhs) +{ +#define MACRO_2(EXPR) EXPR + + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + ~~~~^~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_2 (MACRO_2 (lhs == rhs))); + ^~~~~~~ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_2(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_2 +} + +void test_within_macro_3 (int lhs, int rhs) +{ +#define MACRO_3(EXPR) EXPR + + __emit_expression_range (0, MACRO_3 (lhs) == MACRO_3 (rhs)); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_3 (lhs) == MACRO_3 (rhs)); + ^ + { dg-end-multiline-output "" } */ + +#undef MACRO_3 +} + +void test_within_macro_4 (int lhs, int rhs) +{ +#define MACRO_4(EXPR) EXPR + + __emit_expression_range (0, MACRO_4 (MACRO_4 (lhs) == MACRO_4 (rhs))); + +/* { dg-warning "range" "" { target *-*-* } .-2 } */ +/* { dg-begin-multiline-output "" } + __emit_expression_range (0, MACRO_4 (MACRO_4 (lhs) == MACRO_4 (rhs))); + ^ + { dg-end-multiline-output "" } */ +/* { dg-begin-multiline-output "" } + #define MACRO_4(EXPR) EXPR + ^~~~ + { dg-end-multiline-output "" } */ + +#undef MACRO_4 +} + diff --git a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c index 7c54214..76684f7 100644 --- a/gcc/testsuite/gcc.dg/spellcheck-fields-2.c +++ b/gcc/testsuite/gcc.dg/spellcheck-fields-2.c @@ -28,7 +28,7 @@ int test_macro (union u *ptr) /* { dg-begin-multiline-output "" } #define FIELD colour - ^ + ^~~~~~ { dg-end-multiline-output "" } */ /* { dg-begin-multiline-output "" } diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h index f5c19e3..e696041 100644 --- a/libcpp/include/line-map.h +++ b/libcpp/include/line-map.h @@ -1905,6 +1905,15 @@ void linemap_dump (FILE *, struct line_maps *, unsigned, bool); specifies how many macro maps to dump. */ void line_table_dump (FILE *, struct line_maps *, unsigned int, unsigned int); +/* An enum for distinguishing the various parts within a source_location. */ + +enum location_aspect +{ + LOCATION_ASPECT_CARET, + LOCATION_ASPECT_START, + LOCATION_ASPECT_FINISH +}; + /* The rich_location class requires a way to expand source_location instances. We would directly use expand_location_to_spelling_point, which is implemented in gcc/input.c, but we also need to use it for rich_location @@ -1912,6 +1921,7 @@ void line_table_dump (FILE *, struct line_maps *, unsigned int, unsigned int); Hence we require client code of libcpp to implement the following symbol. */ extern expanded_location -linemap_client_expand_location_to_spelling_point (source_location ); +linemap_client_expand_location_to_spelling_point (source_location, + enum location_aspect); #endif /* !LIBCPP_LINE_MAP_H */ diff --git a/libcpp/line-map.c b/libcpp/line-map.c index 7ba003a..1c1acc8 100644 --- a/libcpp/line-map.c +++ b/libcpp/line-map.c @@ -2065,7 +2065,8 @@ rich_location::get_expanded_location (unsigned int idx) if (!m_have_expanded_location) { m_expanded_location - = linemap_client_expand_location_to_spelling_point (get_loc (0)); + = linemap_client_expand_location_to_spelling_point + (get_loc (0), LOCATION_ASPECT_CARET); if (m_column_override) m_expanded_location.column = m_column_override; m_have_expanded_location = true; @@ -2074,7 +2075,8 @@ rich_location::get_expanded_location (unsigned int idx) return m_expanded_location; } else - return linemap_client_expand_location_to_spelling_point (get_loc (idx)); + return linemap_client_expand_location_to_spelling_point + (get_loc (idx), LOCATION_ASPECT_CARET); } /* Set the column of the primary location, with 0 meaning @@ -2330,9 +2332,11 @@ rich_location::maybe_add_fixit (source_location start, /* Only allow fix-it hints that affect a single line in one file. Compare the end-points. */ expanded_location exploc_start - = linemap_client_expand_location_to_spelling_point (start); + = linemap_client_expand_location_to_spelling_point (start, + LOCATION_ASPECT_START); expanded_location exploc_next_loc - = linemap_client_expand_location_to_spelling_point (next_loc); + = linemap_client_expand_location_to_spelling_point (next_loc, + LOCATION_ASPECT_START); /* They must be within the same file... */ if (exploc_start.file != exploc_next_loc.file) { @@ -2406,13 +2410,15 @@ bool fixit_hint::affects_line_p (const char *file, int line) const { expanded_location exploc_start - = linemap_client_expand_location_to_spelling_point (m_start); + = linemap_client_expand_location_to_spelling_point (m_start, + LOCATION_ASPECT_START); if (file != exploc_start.file) return false; if (line < exploc_start.line) return false; expanded_location exploc_next_loc - = linemap_client_expand_location_to_spelling_point (m_next_loc); + = linemap_client_expand_location_to_spelling_point (m_next_loc, + LOCATION_ASPECT_START); if (file != exploc_next_loc.file) return false; if (line > exploc_next_loc.line) -- 1.8.5.3