[C++] PR 89875 ("[7/8/9/10 Regression] invalid typeof reference to a member of an incomplete struct accepted at function scope")

Message ID 7d7ad6b4-4877-98e0-9daa-a649089f12fa@oracle.com
State New
Headers show
Series
  • [C++] PR 89875 ("[7/8/9/10 Regression] invalid typeof reference to a member of an incomplete struct accepted at function scope")
Related show

Commit Message

Paolo Carlini May 10, 2019, 2:29 p.m.
Hi,

a while ago Martin noticed that an unintended consequence of an old 
tweak of mine - which avoided redundant error messages emitted from 
cp_parser_init_declarator - is that, in some cases, we started accepting 
ill-formed typeofs. Luckily, decltype isn't affected and that points to 
the real issue: by the time that place in cp_parser_init_declarator is 
reached, for a decltype version we already emitted a correct error 
message. Thus I think the right way to fix the problem is simply 
committing to tentative parse when, toward the end of 
cp_parser_sizeof_operand we know that we must be looking at a (possibly 
ill-formed) expression. Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////
/cp
2019-05-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89875
	* parser.c (cp_parser_sizeof_operand): When the type-id production
	did not work out commit to the tentative parse.

/testsuite
2019-05-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/89875
	* g++.dg/cpp0x/decltype-pr66548.C: Remove xfail.
	* g++.dg/template/sizeof-template-argument.C: Adjust expected error.

Comments

Paolo Carlini May 24, 2019, 8:44 p.m. | #1
Hi,

gently pinging this...

On 10/05/19 16:29, Paolo Carlini wrote:
> Hi,

>

> a while ago Martin noticed that an unintended consequence of an old 

> tweak of mine - which avoided redundant error messages emitted from 

> cp_parser_init_declarator - is that, in some cases, we started 

> accepting ill-formed typeofs. Luckily, decltype isn't affected and 

> that points to the real issue: by the time that place in 

> cp_parser_init_declarator is reached, for a decltype version we 

> already emitted a correct error message. Thus I think the right way to 

> fix the problem is simply committing to tentative parse when, toward 

> the end of cp_parser_sizeof_operand we know that we must be looking at 

> a (possibly ill-formed) expression. Tested x86_64-linux.


     https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00501.html

Thanks, Paolo.

PS: To be honest, not knowing the exact rules, I would not consider this 
issue a P2 - in particular considering that typeof is legacy and is 
affected but all sorts of weird issues - but that's what it is ;)
Jason Merrill May 28, 2019, 2:47 p.m. | #2
On 5/10/19 10:29 AM, Paolo Carlini wrote:
> Hi,

> 

> a while ago Martin noticed that an unintended consequence of an old 

> tweak of mine - which avoided redundant error messages emitted from 

> cp_parser_init_declarator - is that, in some cases, we started accepting 

> ill-formed typeofs. Luckily, decltype isn't affected and that points to 

> the real issue: by the time that place in cp_parser_init_declarator is 

> reached, for a decltype version we already emitted a correct error 

> message. Thus I think the right way to fix the problem is simply 

> committing to tentative parse when, toward the end of 

> cp_parser_sizeof_operand we know that we must be looking at a (possibly 

> ill-formed) expression. Tested x86_64-linux.


The problem with calling cp_parser_commit_to_tentative_parse here is 
that the tentative parse you're committing to is for the enclosing 
scope, which is trying to decide whether e.g. we're parsing a 
declaration or expression.  If the operand of typeof is a well-formed 
expression, and the larger context is an expression, this will break.

Better, I think, to commit and re-parse only if you actually encounter 
an error.

Alternately, cp_parser_decltype_expr deals with this by using a 
tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 
the same, but that seems like a bigger hammer.

Jason
Paolo Carlini May 28, 2019, 8:44 p.m. | #3
Hi,

On 28/05/19 16:47, Jason Merrill wrote:
> On 5/10/19 10:29 AM, Paolo Carlini wrote:

>> Hi,

>>

>> a while ago Martin noticed that an unintended consequence of an old 

>> tweak of mine - which avoided redundant error messages emitted from 

>> cp_parser_init_declarator - is that, in some cases, we started 

>> accepting ill-formed typeofs. Luckily, decltype isn't affected and 

>> that points to the real issue: by the time that place in 

>> cp_parser_init_declarator is reached, for a decltype version we 

>> already emitted a correct error message. Thus I think the right way 

>> to fix the problem is simply committing to tentative parse when, 

>> toward the end of cp_parser_sizeof_operand we know that we must be 

>> looking at a (possibly ill-formed) expression. Tested x86_64-linux.

>

> The problem with calling cp_parser_commit_to_tentative_parse here is 

> that the tentative parse you're committing to is for the enclosing 

> scope, which is trying to decide whether e.g. we're parsing a 

> declaration or expression.  If the operand of typeof is a well-formed 

> expression, and the larger context is an expression, this will break.

>

> Better, I think, to commit and re-parse only if you actually encounter 

> an error.

>

> Alternately, cp_parser_decltype_expr deals with this by using a 

> tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 

> the same, but that seems like a bigger hammer.


Today I spent quite a bit of time on this and eventually decided to 
follow the example of decltype as closely as possible. Then I started 
tweaking those drafts which laready passed the testsuite and after a 
while ended up with the below, rather close to the current code, in 
fact. Testing !cp_parser_error_occurred and in case calling 
cp_parser_abort_tentative_parse by hand (closer to the decltype example) 
also works. What do you think? Thanks, Paolo.

///////////////////////
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 271692)
+++ cp/parser.c	(working copy)
@@ -28942,6 +28942,8 @@ cp_parser_sizeof_operand (cp_parser* parser, enum
     {
       tree type = NULL_TREE;
 
+      tentative_firewall firewall (parser);
+
       /* We can't be sure yet whether we're looking at a type-id or an
 	 expression.  */
       cp_parser_parse_tentatively (parser);
@@ -28969,11 +28971,15 @@ cp_parser_sizeof_operand (cp_parser* parser, enum
       /* If all went well, then we're done.  */
       if (cp_parser_parse_definitely (parser))
 	expr = type;
+      else
+	{
+	  /* Commit to the tentative_firewall so we get syntax errors.  */
+	  cp_parser_commit_to_tentative_parse (parser);
+
+	  expr = cp_parser_unary_expression (parser);
+	}
     }
-
-  /* If the type-id production did not work out, then we must be
-     looking at the unary-expression production.  */
-  if (!expr)
+  else
     expr = cp_parser_unary_expression (parser);
 
   /* Go back to evaluating expressions.  */
Index: testsuite/g++.dg/cpp0x/decltype-pr66548.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-pr66548.C	(revision 271692)
+++ testsuite/g++.dg/cpp0x/decltype-pr66548.C	(working copy)
@@ -11,7 +11,7 @@ struct Meow {};
 
 void f ()
 {
-  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" "pr89875" { xfail c++98_only } }
+  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" }
   (void)&d;
 }
 
Index: testsuite/g++.dg/template/sizeof-template-argument.C
===================================================================
--- testsuite/g++.dg/template/sizeof-template-argument.C	(revision 271692)
+++ testsuite/g++.dg/template/sizeof-template-argument.C	(working copy)
@@ -3,9 +3,9 @@
 
 template<int> struct A {};
 
-template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
-template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
 int a;
Jason Merrill May 29, 2019, 4:20 p.m. | #4
On 5/28/19 4:44 PM, Paolo Carlini wrote:
> Hi,

> 

> On 28/05/19 16:47, Jason Merrill wrote:

>> On 5/10/19 10:29 AM, Paolo Carlini wrote:

>>> Hi,

>>>

>>> a while ago Martin noticed that an unintended consequence of an old 

>>> tweak of mine - which avoided redundant error messages emitted from 

>>> cp_parser_init_declarator - is that, in some cases, we started 

>>> accepting ill-formed typeofs. Luckily, decltype isn't affected and 

>>> that points to the real issue: by the time that place in 

>>> cp_parser_init_declarator is reached, for a decltype version we 

>>> already emitted a correct error message. Thus I think the right way 

>>> to fix the problem is simply committing to tentative parse when, 

>>> toward the end of cp_parser_sizeof_operand we know that we must be 

>>> looking at a (possibly ill-formed) expression. Tested x86_64-linux.

>>

>> The problem with calling cp_parser_commit_to_tentative_parse here is 

>> that the tentative parse you're committing to is for the enclosing 

>> scope, which is trying to decide whether e.g. we're parsing a 

>> declaration or expression.  If the operand of typeof is a well-formed 

>> expression, and the larger context is an expression, this will break.

>>

>> Better, I think, to commit and re-parse only if you actually encounter 

>> an error.

>>

>> Alternately, cp_parser_decltype_expr deals with this by using a 

>> tentative firewall and CPP_DECLTYPE; cp_parser_sizeof_operand could do 

>> the same, but that seems like a bigger hammer.

> 

> Today I spent quite a bit of time on this and eventually decided to 

> follow the example of decltype as closely as possible. Then I started 

> tweaking those drafts which laready passed the testsuite and after a 

> while ended up with the below, rather close to the current code, in 

> fact. Testing !cp_parser_error_occurred and in case calling 

> cp_parser_abort_tentative_parse by hand (closer to the decltype example) 

> also works. What do you think? Thanks, Paolo.


OK.

Jason

Patch

Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 271059)
+++ cp/parser.c	(working copy)
@@ -28998,7 +28998,11 @@  cp_parser_sizeof_operand (cp_parser* parser, enum
   /* If the type-id production did not work out, then we must be
      looking at the unary-expression production.  */
   if (!expr)
-    expr = cp_parser_unary_expression (parser);
+    {
+      cp_parser_commit_to_tentative_parse (parser);
+      
+      expr = cp_parser_unary_expression (parser);
+    }
 
   /* Go back to evaluating expressions.  */
   --cp_unevaluated_operand;
Index: testsuite/g++.dg/cpp0x/decltype-pr66548.C
===================================================================
--- testsuite/g++.dg/cpp0x/decltype-pr66548.C	(revision 271059)
+++ testsuite/g++.dg/cpp0x/decltype-pr66548.C	(working copy)
@@ -11,7 +11,7 @@  struct Meow {};
 
 void f ()
 {
-  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" "pr89875" { xfail c++98_only } }
+  decltype (Meow.purr ()) d;   // { dg-error "expected primary-expression" }
   (void)&d;
 }
 
Index: testsuite/g++.dg/template/sizeof-template-argument.C
===================================================================
--- testsuite/g++.dg/template/sizeof-template-argument.C	(revision 271059)
+++ testsuite/g++.dg/template/sizeof-template-argument.C	(working copy)
@@ -3,9 +3,9 @@ 
 
 template<int> struct A {};
 
-template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct B : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
-template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "template argument" } */
+template<typename> struct C : A <sizeof(=)> {}; /* { dg-error "expected primary-expression" } */
 
 int a;