| Message ID | 01020191ffe7d291-0ceb3852-2428-49cc-97c5-7c6e8072ec43-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Avoid "infinite parsing" because of cp_parser_decltype [PR114858] | expand |
Friendly ping. Thanks! On 17 Sep 2024, at 14:14, Simon Martin wrote: > The invalid test case in this PR highlights a bad interaction between > the tentative_firewall and error recovery in cp_parser_decltype: the > firewall makes cp_parser_skip_to_closing_parenthesis a no-op, and the > parser does not make any progress, running "forever". > > This patch calls cp_parser_commit_to_tentative_parse before initiating > error recovery. > > Successfully tested on x86_64-pc-linux-gnu. > > PR c++/114858 > > gcc/cp/ChangeLog: > > * parser.cc (cp_parser_decltype): Commit tentative parse before > initiating error recovery. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/decltype10.C: Adjust test expectation. > * g++.dg/cpp2a/pr114858.C: New test. > --- > gcc/cp/parser.cc | 3 +++ > gcc/testsuite/g++.dg/cpp0x/decltype10.C | 2 ++ > gcc/testsuite/g++.dg/cpp2a/pr114858.C | 25 > +++++++++++++++++++++++++ > 3 files changed, 30 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/pr114858.C > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index 4dd9474cf60..3a7c5ffe4c8 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -17508,6 +17508,9 @@ cp_parser_decltype (cp_parser *parser) > /* Parse to the closing `)'. */ > if (expr == error_mark_node || !parens.require_close (parser)) > { > + /* Commit to the tentative_firewall so we actually skip to the > closing > + parenthesis. */ > + cp_parser_commit_to_tentative_parse (parser); > cp_parser_skip_to_closing_parenthesis (parser, true, false, > /*consume_paren=*/true); > expr = error_mark_node; > diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype10.C > b/gcc/testsuite/g++.dg/cpp0x/decltype10.C > index fe7247269f5..bd606e325d4 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/decltype10.C > +++ b/gcc/testsuite/g++.dg/cpp0x/decltype10.C > @@ -7,3 +7,5 @@ template<int> struct A > }; > > template<int N> int A<N>::i(decltype (A::i; // { dg-error "expected" > } > + > +// { dg-excess-errors "" } > diff --git a/gcc/testsuite/g++.dg/cpp2a/pr114858.C > b/gcc/testsuite/g++.dg/cpp2a/pr114858.C > new file mode 100644 > index 00000000000..6ffde4c3a2c > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/pr114858.C > @@ -0,0 +1,25 @@ > +// PR c++/114858 > +// { dg-do compile { target c++20 } } > +// { dg-timeout 2 } > + > +template <class F> void g(F); > +template <class... A> > +auto h(A &&... a) -> decltype( > + decltype(g< // { dg-error "expected primary-expression" } > + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< > + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< > + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< > + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< > + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< > + decltype(g<decltype(g< > + decltype()>)(a)...) > +{ > + h([] {}); > +} > + > +int main() { > + h(); > + return 0; > +} > + > +// { dg-excess-errors "" } > -- > 2.44.0
On 9/17/24 8:14 AM, Simon Martin wrote: > The invalid test case in this PR highlights a bad interaction between > the tentative_firewall and error recovery in cp_parser_decltype: the > firewall makes cp_parser_skip_to_closing_parenthesis a no-op, and the > parser does not make any progress, running "forever". > > This patch calls cp_parser_commit_to_tentative_parse before initiating > error recovery. > > Successfully tested on x86_64-pc-linux-gnu. > > PR c++/114858 > > gcc/cp/ChangeLog: > > * parser.cc (cp_parser_decltype): Commit tentative parse before > initiating error recovery. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/decltype10.C: Adjust test expectation. > * g++.dg/cpp2a/pr114858.C: New test. > --- > gcc/cp/parser.cc | 3 +++ > gcc/testsuite/g++.dg/cpp0x/decltype10.C | 2 ++ > gcc/testsuite/g++.dg/cpp2a/pr114858.C | 25 +++++++++++++++++++++++++ > 3 files changed, 30 insertions(+) > create mode 100644 gcc/testsuite/g++.dg/cpp2a/pr114858.C > > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc > index 4dd9474cf60..3a7c5ffe4c8 100644 > --- a/gcc/cp/parser.cc > +++ b/gcc/cp/parser.cc > @@ -17508,6 +17508,9 @@ cp_parser_decltype (cp_parser *parser) > /* Parse to the closing `)'. */ > if (expr == error_mark_node || !parens.require_close (parser)) > { > + /* Commit to the tentative_firewall so we actually skip to the closing > + parenthesis. */ > + cp_parser_commit_to_tentative_parse (parser); I don't think this is right. Earlier in cp_parser_decltype I see > /* If in_declarator_p, a reparse as an expression might succeed (60361). > Otherwise, commit now for better diagnostics. */ > if (cp_parser_uncommitted_to_tentative_parse_p (parser) > && !parser->in_declarator_p) > cp_parser_commit_to_topmost_tentative_parse (parser); Here we're in a declarator, so we didn't commit at that point. And we still don't want to commit if parsing fails; as the comment says, when reparsing as an expression-statement it might work. Though there seems not to be a testcase for that... In trying to come up with a testcase, I wrote this one that already fails because the error doesn't happen until after the decltype, so we memorize the wrong result: struct Helper { Helper(int, ...); }; template <class T> struct C; template<> struct C<char> {}; char A = 1; Helper testFail(int(A), C<decltype(A)>{}); // { dg-bogus "C<int>" } So in the long term we need to overhaul this code to handle reparsing even without a syntax error. But it's not a high priority. Getting back to your patch, I think the problem is in cp_parser_simple_type_specifier: > case RID_DECLTYPE: > /* Since DR 743, decltype can either be a simple-type-specifier by > itself or begin a nested-name-specifier. Parsing it will replace > it with a CPP_DECLTYPE, so just rewind and let the CPP_DECLTYPE > handling below decide what to do. */ > cp_parser_decltype (parser); > cp_lexer_set_token_position (parser->lexer, token); > break; This assumes that cp_parser_decltype will always succeed, which is wrong. We need to check whether the token actually became CPP_DECLTYPE and parser_error if not. Jason
Hi Jason, On 30 Sep 2024, at 20:56, Jason Merrill wrote: > On 9/17/24 8:14 AM, Simon Martin wrote: >> The invalid test case in this PR highlights a bad interaction between >> the tentative_firewall and error recovery in cp_parser_decltype: the >> firewall makes cp_parser_skip_to_closing_parenthesis a no-op, and the >> parser does not make any progress, running "forever". >> >> This patch calls cp_parser_commit_to_tentative_parse before >> initiating >> error recovery. >> >> Successfully tested on x86_64-pc-linux-gnu. >> >> PR c++/114858 >> >> gcc/cp/ChangeLog: >> >> * parser.cc (cp_parser_decltype): Commit tentative parse before >> initiating error recovery. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/cpp0x/decltype10.C: Adjust test expectation. >> * g++.dg/cpp2a/pr114858.C: New test. >> --- >> gcc/cp/parser.cc | 3 +++ >> gcc/testsuite/g++.dg/cpp0x/decltype10.C | 2 ++ >> gcc/testsuite/g++.dg/cpp2a/pr114858.C | 25 >> +++++++++++++++++++++++++ >> 3 files changed, 30 insertions(+) >> create mode 100644 gcc/testsuite/g++.dg/cpp2a/pr114858.C >> >> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc >> index 4dd9474cf60..3a7c5ffe4c8 100644 >> --- a/gcc/cp/parser.cc >> +++ b/gcc/cp/parser.cc >> @@ -17508,6 +17508,9 @@ cp_parser_decltype (cp_parser *parser) >> /* Parse to the closing `)'. */ >> if (expr == error_mark_node || !parens.require_close (parser)) >> { >> + /* Commit to the tentative_firewall so we actually skip to the >> closing >> + parenthesis. */ >> + cp_parser_commit_to_tentative_parse (parser); > > I don't think this is right. > > Earlier in cp_parser_decltype I see > >> /* If in_declarator_p, a reparse as an expression might succeed >> (60361). >> Otherwise, commit now for better diagnostics. */ >> if (cp_parser_uncommitted_to_tentative_parse_p (parser) >> && !parser->in_declarator_p) >> cp_parser_commit_to_topmost_tentative_parse (parser); > > Here we're in a declarator, so we didn't commit at that point. And we > still don't want to commit if parsing fails; as the comment says, when > reparsing as an expression-statement it might work. Though there > seems not to be a testcase for that... Right, understood. I’ll see if I can come up with something in a follow-up patch. > > In trying to come up with a testcase, I wrote this one that already > fails because the error doesn't happen until after the decltype, so we > memorize the wrong result: > > struct Helper { Helper(int, ...); }; > template <class T> struct C; > template<> struct C<char> {}; > char A = 1; > Helper testFail(int(A), C<decltype(A)>{}); // { dg-bogus "C<int>" } > > So in the long term we need to overhaul this code to handle reparsing > even without a syntax error. But it's not a high priority. Nice; I’ll open a PR for that, and try to take a stab at it. > > Getting back to your patch, I think the problem is in > cp_parser_simple_type_specifier: > >> case RID_DECLTYPE: >> /* Since DR 743, decltype can either be a simple-type-specifier >> by >> itself or begin a nested-name-specifier. Parsing it will replace >> it >> with a CPP_DECLTYPE, so just rewind and let the CPP_DECLTYPE >> handling >> below decide what to do. */ >> cp_parser_decltype (parser); >> cp_lexer_set_token_position (parser->lexer, token); >> break; > > This assumes that cp_parser_decltype will always succeed, which is > wrong. We need to check whether the token actually became > CPP_DECLTYPE and parser_error if not. We should definitely to this. Note however that it’s not sufficient to fix the test case: compilation time still explodes because for constantly re-parse “brain dead” template ids (unterminated parameter list). The attached patch fixes both issues, and has been successfully tested on x86_64-pc-linux-gnu. Ok for trunk? Thanks, Simon From 61fadd961442b919281c865d0db90fb4ed6265aa Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Mon, 16 Sep 2024 10:14:46 +0200 Subject: [PATCH] c++: Avoid "infinite parsing" because of cp_parser_decltype [PR114858] The invalid test case in this PR highlights two deficiencies: 1. cp_parser_simple_type_specifier assumes that parsing a decltype always succeeds 2. cp_parser_template_id does not turn the sequence of tokens into a CPP_TEMPLATE_ID even in case we're sure that no reparse will ever succeed (e.g. the template parameter list is not terminated). So for each "decltype level" in the test case, we try to parse the decltype expression as an id-expression, then a postfix-expression, then an expression, failing every time, and the compilation time explodes. This patches addresses issue #1 by checking whether the decltype token was turned into a CPP_DECLTYPE, and #2 by still producing a CPP_TEMPLATE_ID if the template parameter list is not terminated, to avoid reparsing ad nauseam. Successfully tested on x86_64-pc-linux-gnu. PR c++/114858 gcc/cp/ChangeLog: * parser.cc (cp_parser_template_id): Produce a CPP_TEMPLATE_ID if the parameter list is not terminated. (cp_parser_simple_type_specifier): cp_parser_decltype not producing a CPP_DECLTYPE is an error. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/pr33839.C: Adjust test expectation. * g++.dg/cpp2a/pr114858.C: New test. --- gcc/cp/parser.cc | 26 +++++++++++++++++++++----- gcc/testsuite/g++.dg/cpp0x/pr33839.C | 2 +- gcc/testsuite/g++.dg/cpp2a/pr114858.C | 26 ++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/g++.dg/cpp2a/pr114858.C diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 9d31a975dcf..251b1cc5c47 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -19133,6 +19133,7 @@ cp_parser_template_id (cp_parser *parser, cp_token_position start_of_id = 0; cp_token *next_token = NULL, *next_token_2 = NULL; bool is_identifier; + bool unrecoverable_error_p = false; /* If the next token corresponds to a template-id, there is no need to reparse it. */ @@ -19271,6 +19272,14 @@ cp_parser_template_id (cp_parser *parser, pop_deferring_access_checks (); return error_mark_node; } + else if (cp_parser_error_occurred (parser)) + { + cp_token *prev = cp_lexer_safe_previous_token (parser->lexer); + if (!(prev && prev->type == CPP_GREATER)) + /* If the parameter list is not closed, no reparse will ever + succeed. */ + unrecoverable_error_p = true; + } } /* Set the location to be of the form: @@ -19341,11 +19350,12 @@ cp_parser_template_id (cp_parser *parser, error messages about problems during instantiation of the template. */ if (start_of_id - /* Don't do this if we had a parse error in a declarator; re-parsing + /* Don't do this if we had a parse error in a declarator (unless it's + unrecoverable, e.g. the parameter list is unterminated); re-parsing might succeed if a name changes meaning (60361). */ - && !(cp_parser_error_occurred (parser) - && cp_parser_parsing_tentatively (parser) - && parser->in_declarator_p)) + && (!(cp_parser_error_occurred (parser) + && parser->in_declarator_p + && !unrecoverable_error_p))) { /* Reset the contents of the START_OF_ID token. */ token->type = CPP_TEMPLATE_ID; @@ -20636,7 +20646,13 @@ cp_parser_simple_type_specifier (cp_parser* parser, it with a CPP_DECLTYPE, so just rewind and let the CPP_DECLTYPE handling below decide what to do. */ cp_parser_decltype (parser); - cp_lexer_set_token_position (parser->lexer, token); + if (token->type == CPP_DECLTYPE) + cp_lexer_set_token_position (parser->lexer, token); + else + { + cp_parser_error(parser, "invalid %<decltype%> type"); + type = NULL_TREE; + } break; case RID_TYPEOF: diff --git a/gcc/testsuite/g++.dg/cpp0x/pr33839.C b/gcc/testsuite/g++.dg/cpp0x/pr33839.C index 50bcfe81cf5..3c3487cf206 100644 --- a/gcc/testsuite/g++.dg/cpp0x/pr33839.C +++ b/gcc/testsuite/g++.dg/cpp0x/pr33839.C @@ -3,6 +3,6 @@ template<int> struct A; void foo() { - __decltype A<0>; // { dg-error "invalid declarator|expected" } + __decltype A<0>; // { dg-error "does not declare anything|expected" } __decltype (A<0>); // { dg-error "must be an expression" } } diff --git a/gcc/testsuite/g++.dg/cpp2a/pr114858.C b/gcc/testsuite/g++.dg/cpp2a/pr114858.C new file mode 100644 index 00000000000..3a44c47ca09 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/pr114858.C @@ -0,0 +1,26 @@ +// PR c++/114858 +// { dg-do compile { target c++20 } } +// { dg-additional-options "-fno-diagnostics-show-caret" } +// { dg-timeout 2 } + +template <class F> void g(F); +template <class... A> +auto h(A &&... a) -> decltype( + decltype(g< // { dg-error "expected primary-expression" } + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g< + decltype()>)(a)...) +{ + h([] {}); +} + +int main() { + h(); + return 0; +} + +// { dg-excess-errors "" }
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc index 4dd9474cf60..3a7c5ffe4c8 100644 --- a/gcc/cp/parser.cc +++ b/gcc/cp/parser.cc @@ -17508,6 +17508,9 @@ cp_parser_decltype (cp_parser *parser) /* Parse to the closing `)'. */ if (expr == error_mark_node || !parens.require_close (parser)) { + /* Commit to the tentative_firewall so we actually skip to the closing + parenthesis. */ + cp_parser_commit_to_tentative_parse (parser); cp_parser_skip_to_closing_parenthesis (parser, true, false, /*consume_paren=*/true); expr = error_mark_node; diff --git a/gcc/testsuite/g++.dg/cpp0x/decltype10.C b/gcc/testsuite/g++.dg/cpp0x/decltype10.C index fe7247269f5..bd606e325d4 100644 --- a/gcc/testsuite/g++.dg/cpp0x/decltype10.C +++ b/gcc/testsuite/g++.dg/cpp0x/decltype10.C @@ -7,3 +7,5 @@ template<int> struct A }; template<int N> int A<N>::i(decltype (A::i; // { dg-error "expected" } + +// { dg-excess-errors "" } diff --git a/gcc/testsuite/g++.dg/cpp2a/pr114858.C b/gcc/testsuite/g++.dg/cpp2a/pr114858.C new file mode 100644 index 00000000000..6ffde4c3a2c --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp2a/pr114858.C @@ -0,0 +1,25 @@ +// PR c++/114858 +// { dg-do compile { target c++20 } } +// { dg-timeout 2 } + +template <class F> void g(F); +template <class... A> +auto h(A &&... a) -> decltype( + decltype(g< // { dg-error "expected primary-expression" } + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g<decltype(g<decltype(g<decltype(g<decltype(g< + decltype(g<decltype(g< + decltype()>)(a)...) +{ + h([] {}); +} + +int main() { + h(); + return 0; +} + +// { dg-excess-errors "" }