Re: [PATCH] diagnostics: fix end-points of ranges within macros (PR c++/79300)
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 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. jeff
Re: [PATCH] diagnostics: fix end-points of ranges within macros (PR c++/79300)
On 02/03/2017 01:52 PM, David Malcolm wrote: IIRC you had another issue that was borderline, but which we did want to move forward on gcc-7. The reported testcase was a problem with a new warning, but the underlying issue could (in theory) be tripped for existing warnings. RIght? I think you're talking about PR preprocessor/79210. I found a trivial way to turn the reproducer for that one into one that ICE-d gcc 7 with input that gcc 6 could handle, so I marked it as "[7 Regression]", committed a fix to trunk, and marked it as RESOLVED FIXED. That's the one :-) Perfect. Thanks. jeff
Re: [PATCH] diagnostics: fix end-points of ranges within macros (PR c++/79300)
On Fri, 2017-02-03 at 13:28 -0700, 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 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. > I'd suggest waiting. OK; I plan to self-approve this one in stage 1. > IIRC you had another issue that was borderline, > but which we did want to move forward on gcc-7. The reported > testcase > was a problem with a new warning, but the underlying issue could (in > theory) be tripped for existing warnings. RIght? I think you're talking about PR preprocessor/79210. I found a trivial way to turn the reproducer for that one into one that ICE-d gcc 7 with input that gcc 6 could handle, so I marked it as "[7 Regression]", committed a fix to trunk, and marked it as RESOLVED FIXED. Dave
Re: [PATCH] diagnostics: fix end-points of ranges within macros (PR c++/79300)
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 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. I'd suggest waiting. IIRC you had another issue that was borderline, but which we did want to move forward on gcc-7. The reported testcase was a problem with a new warning, but the underlying issue could (in theory) be tripped for existing warnings. RIght? Jeff
[PATCH] diagnostics: fix end-points of ranges within macros (PR c++/79300)
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 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. --- 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 | 15 ++-- 11 files changed, 234 insertions(+), 24 deletions(-) diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index 5c386ae..a0914ef 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -784,11 +784,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 +
