| Message ID | 20200710154359.2172569-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix tentative parsing of enum-specifier [PR96077] | expand |
On Fri, Jul 10, 2020 at 11:43:59AM -0400, Marek Polacek via Gcc-patches wrote: > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 528b41b7170..ee6a956aea9 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -19412,7 +19412,12 @@ cp_parser_enum_specifier (cp_parser* parser) > "ISO C++ forbids empty unnamed enum"); > } > else > - cp_parser_enumerator_list (parser, type); > + { > + /* We've seen a '}' so we know we're in an enum-specifier. Shouldn't that be '{' instead? > + Commit to any tentative parse to get syntax errors. */ > + cp_parser_commit_to_tentative_parse (parser); > + cp_parser_enumerator_list (parser, type); > + } Jakub
On Fri, Jul 10, 2020 at 05:53:21PM +0200, Jakub Jelinek wrote: > On Fri, Jul 10, 2020 at 11:43:59AM -0400, Marek Polacek via Gcc-patches wrote: > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > > index 528b41b7170..ee6a956aea9 100644 > > --- a/gcc/cp/parser.c > > +++ b/gcc/cp/parser.c > > @@ -19412,7 +19412,12 @@ cp_parser_enum_specifier (cp_parser* parser) > > "ISO C++ forbids empty unnamed enum"); > > } > > else > > - cp_parser_enumerator_list (parser, type); > > + { > > + /* We've seen a '}' so we know we're in an enum-specifier. > > Shouldn't that be '{' instead? Yes, fixed in my local branch. Thanks! Marek
On 7/10/20 11:43 AM, Marek Polacek via Gcc-patches wrote: > Here's an interesting issue: in this code a ) is missing: > > enum { E = (2 } e; > > but we compile the code anyway, and E is set to 0 in build_enumerator, > which is sneaky. > > The problem is that cp_parser_enum_specifier parses tentatively, because > when we see the enum keyword, we don't know yet if we'll find an > enum-specifier, opaque-enum-declaration, or elaborated-enum-specifier. > > In this test when we call cp_parser_enumerator_list we're still parsing > tentatively, and as a consequence, parens.require_close (parser) in > cp_parser_primary_expression doesn't report any errors. But we only go > on to parse the enumerator-list after we've seen a {, at which point we > might as well commit -- we know we're dealing with an enum-specifier. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? ok, (with the typo Jakub noticed fixed :) does this also fix 95288? > gcc/cp/ChangeLog: > > PR c++/96077 > * parser.c (cp_parser_enum_specifier): Commit to tentative parse > after we've seen an opening brace. > > gcc/testsuite/ChangeLog: > > PR c++/96077 > * g++.dg/parse/enum14.C: New test. nathan
On Mon, Jul 13, 2020 at 10:08:52AM -0400, Nathan Sidwell wrote: > On 7/10/20 11:43 AM, Marek Polacek via Gcc-patches wrote: > > Here's an interesting issue: in this code a ) is missing: > > > > enum { E = (2 } e; > > > > but we compile the code anyway, and E is set to 0 in build_enumerator, > > which is sneaky. > > > > The problem is that cp_parser_enum_specifier parses tentatively, because > > when we see the enum keyword, we don't know yet if we'll find an > > enum-specifier, opaque-enum-declaration, or elaborated-enum-specifier. > > > > In this test when we call cp_parser_enumerator_list we're still parsing > > tentatively, and as a consequence, parens.require_close (parser) in > > cp_parser_primary_expression doesn't report any errors. But we only go > > on to parse the enumerator-list after we've seen a {, at which point we > > might as well commit -- we know we're dealing with an enum-specifier. > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > ok, (with the typo Jakub noticed fixed :) Thanks. > does this also fix 95288? Hmm, in a way: instead of 95288.C:3:3: error: expected primary-expression before ‘enum’ 3 | enum X | ^~~~ with this patch we say 95288.C:5:8: error: expected ‘}’ before ‘.’ token 5 | a. // DOT | ^ 95288.C:4:5: note: to match this ‘{’ 4 | { | ^ but the rest of the output is the same. Whether that means that 95288 is fixed, I'm not too sure. But since you opened it, if you're happy with the diagnostic with this patch, I'll add the test and close 95288. Marek
On 7/13/20 11:15 AM, Marek Polacek wrote: > On Mon, Jul 13, 2020 at 10:08:52AM -0400, Nathan Sidwell wrote: >> On 7/10/20 11:43 AM, Marek Polacek via Gcc-patches wrote: >>> Here's an interesting issue: in this code a ) is missing: >>> >>> enum { E = (2 } e; >>> >>> but we compile the code anyway, and E is set to 0 in build_enumerator, >>> which is sneaky. >>> >>> The problem is that cp_parser_enum_specifier parses tentatively, because >>> when we see the enum keyword, we don't know yet if we'll find an >>> enum-specifier, opaque-enum-declaration, or elaborated-enum-specifier. >>> >>> In this test when we call cp_parser_enumerator_list we're still parsing >>> tentatively, and as a consequence, parens.require_close (parser) in >>> cp_parser_primary_expression doesn't report any errors. But we only go >>> on to parse the enumerator-list after we've seen a {, at which point we >>> might as well commit -- we know we're dealing with an enum-specifier. >>> >>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >> >> ok, (with the typo Jakub noticed fixed :) > > Thanks. > >> does this also fix 95288? > > Hmm, in a way: instead of > > 95288.C:3:3: error: expected primary-expression before ‘enum’ > 3 | enum X > | ^~~~ > > with this patch we say > > 95288.C:5:8: error: expected ‘}’ before ‘.’ token > 5 | a. // DOT > | ^ > 95288.C:4:5: note: to match this ‘{’ > 4 | { > | ^ > > but the rest of the output is the same. > > Whether that means that 95288 is fixed, I'm not too sure. But since you opened > it, if you're happy with the diagnostic with this patch, I'll add the test and > close 95288. Yes, that's the fix I'd expect. I'm sorry the report didn't make it clear that the function-scope diagnostic is the poor one. nathan
On Mon, Jul 13, 2020 at 12:18:03PM -0400, Nathan Sidwell wrote: > On 7/13/20 11:15 AM, Marek Polacek wrote: > > On Mon, Jul 13, 2020 at 10:08:52AM -0400, Nathan Sidwell wrote: > > > On 7/10/20 11:43 AM, Marek Polacek via Gcc-patches wrote: > > > > Here's an interesting issue: in this code a ) is missing: > > > > > > > > enum { E = (2 } e; > > > > > > > > but we compile the code anyway, and E is set to 0 in build_enumerator, > > > > which is sneaky. > > > > > > > > The problem is that cp_parser_enum_specifier parses tentatively, because > > > > when we see the enum keyword, we don't know yet if we'll find an > > > > enum-specifier, opaque-enum-declaration, or elaborated-enum-specifier. > > > > > > > > In this test when we call cp_parser_enumerator_list we're still parsing > > > > tentatively, and as a consequence, parens.require_close (parser) in > > > > cp_parser_primary_expression doesn't report any errors. But we only go > > > > on to parse the enumerator-list after we've seen a {, at which point we > > > > might as well commit -- we know we're dealing with an enum-specifier. > > > > > > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > > > ok, (with the typo Jakub noticed fixed :) > > > > Thanks. > > > > > does this also fix 95288? > > > > Hmm, in a way: instead of > > > > 95288.C:3:3: error: expected primary-expression before ‘enum’ > > 3 | enum X > > | ^~~~ > > > > with this patch we say > > > > 95288.C:5:8: error: expected ‘}’ before ‘.’ token > > 5 | a. // DOT > > | ^ > > 95288.C:4:5: note: to match this ‘{’ > > 4 | { > > | ^ > > > > but the rest of the output is the same. > > > > Whether that means that 95288 is fixed, I'm not too sure. But since you opened > > it, if you're happy with the diagnostic with this patch, I'll add the test and > > close 95288. > > Yes, that's the fix I'd expect. I'm sorry the report didn't make it clear > that the function-scope diagnostic is the poor one. No worries. I've added the test and closed the PR. Marek
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 528b41b7170..ee6a956aea9 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -19412,7 +19412,12 @@ cp_parser_enum_specifier (cp_parser* parser) "ISO C++ forbids empty unnamed enum"); } else - cp_parser_enumerator_list (parser, type); + { + /* We've seen a '}' so we know we're in an enum-specifier. + Commit to any tentative parse to get syntax errors. */ + cp_parser_commit_to_tentative_parse (parser); + cp_parser_enumerator_list (parser, type); + } /* Consume the final '}'. */ braces.require_close (parser); diff --git a/gcc/testsuite/g++.dg/parse/enum14.C b/gcc/testsuite/g++.dg/parse/enum14.C new file mode 100644 index 00000000000..be09cca5211 --- /dev/null +++ b/gcc/testsuite/g++.dg/parse/enum14.C @@ -0,0 +1,7 @@ +// PR c++/96077 + +int main () +{ + enum { E = (2 } e; // { dg-error "expected" } + enum { F = true ? 2 : (3 /* missing ")" here */ } f; // { dg-error "expected" } +}
Here's an interesting issue: in this code a ) is missing: enum { E = (2 } e; but we compile the code anyway, and E is set to 0 in build_enumerator, which is sneaky. The problem is that cp_parser_enum_specifier parses tentatively, because when we see the enum keyword, we don't know yet if we'll find an enum-specifier, opaque-enum-declaration, or elaborated-enum-specifier. In this test when we call cp_parser_enumerator_list we're still parsing tentatively, and as a consequence, parens.require_close (parser) in cp_parser_primary_expression doesn't report any errors. But we only go on to parse the enumerator-list after we've seen a {, at which point we might as well commit -- we know we're dealing with an enum-specifier. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? gcc/cp/ChangeLog: PR c++/96077 * parser.c (cp_parser_enum_specifier): Commit to tentative parse after we've seen an opening brace. gcc/testsuite/ChangeLog: PR c++/96077 * g++.dg/parse/enum14.C: New test. --- gcc/cp/parser.c | 7 ++++++- gcc/testsuite/g++.dg/parse/enum14.C | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/parse/enum14.C base-commit: c6b7ba5de624f2a17d799bac5ff017cd065ce035