[PATCH] c++: Fix parsing [[]][[]];

[PATCH] c++: Fix parsing [[]][[]];

[Jakub Jelinek]

Hi!

When working on the previous patch I put [[]] [[]] asm (""); into a
testcase, but was surprised it wasn't parsed.
The problem is that when cp_parser_std_attribute_spec returns NULL, it
can mean 2 different things, one is that the next token(s) are neither
[[ nor alignas (in that case the caller should break from the loop),
or when we parsed something like [[]] - it was valid attribute specifier,
but didn't specify any attributes in it.

The following patch fixes that by adding another parameter to differentiate
between the cases, guess another option would be to use some magic
tree value for the break case instead of NULL_TREE (but error_mark_node is
already taken and means something else).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Or shall I go with some magic return which will never happen otherwise?
void_node?

2023-12-05  Jakub Jelinek  <jakub@redhat.com>

	* parser.cc (cp_parser_std_attribute_spec): Add ANY_P argument, set
	what it points to initially to true and only if token is neither
	CPP_OPEN_SQUARE nor RID_ALIGNAS CPP_KEYWORD set it to false.
	(cp_parser_std_attribute_spec_seq): Adjust
	cp_parser_std_attribute_spec caller.  If it returns NULL_TREE and
	any_p is true, continue rather than break.

	* g++.dg/cpp0x/gen-attrs-79.C: New test.

--- gcc/cp/parser.cc.jj	2023-12-04 20:23:53.225009856 +0100
+++ gcc/cp/parser.cc	2023-12-04 20:49:21.160426104 +0100
@@ -2703,7 +2703,7 @@ static tree cp_parser_gnu_attribute_list
 static tree cp_parser_std_attribute
   (cp_parser *, tree);
 static tree cp_parser_std_attribute_spec
-  (cp_parser *);
+  (cp_parser *, bool *);
 static tree cp_parser_std_attribute_spec_seq
   (cp_parser *);
 static size_t cp_parser_skip_std_attribute_spec_seq
@@ -30265,11 +30265,12 @@ void cp_parser_late_contract_condition (
 	 conditional-expression ] ]  */
 
 static tree
-cp_parser_std_attribute_spec (cp_parser *parser)
+cp_parser_std_attribute_spec (cp_parser *parser, bool *any_p)
 {
   tree attributes = NULL_TREE;
   cp_token *token = cp_lexer_peek_token (parser->lexer);
 
+  *any_p = true;
   if (token->type == CPP_OPEN_SQUARE
       && cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_OPEN_SQUARE)
     {
@@ -30342,7 +30343,10 @@ cp_parser_std_attribute_spec (cp_parser
 
       if (token->type != CPP_KEYWORD
 	  || token->keyword != RID_ALIGNAS)
-	return NULL_TREE;
+	{
+	  *any_p = false;
+	  return NULL_TREE;
+	}
 
       cp_lexer_consume_token (parser->lexer);
       maybe_warn_cpp0x (CPP0X_ATTRIBUTES);
@@ -30414,9 +30418,16 @@ cp_parser_std_attribute_spec_seq (cp_par
 
   while (true)
     {
-      tree attr_spec = cp_parser_std_attribute_spec (parser);
+      bool any_p;
+      tree attr_spec = cp_parser_std_attribute_spec (parser, &any_p);
       if (attr_spec == NULL_TREE)
-	break;
+	{
+	  /* Accept [[]][[]]; for which cp_parser_std_attribute_spec
+	     also returns NULL_TREE as there are no attributes.  */
+	  if (any_p)
+	    continue;
+	  break;
+	}
       if (attr_spec == error_mark_node)
 	return error_mark_node;
 
--- gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C.jj	2023-12-04 20:38:35.122574430 +0100
+++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C	2023-12-04 20:38:29.468654143 +0100
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++11 } }
+
+[[]] [[]];
+
+[[]] [[]] void
+foo ()
+{
+  [[]] [[]];
+}

	Jakub

Re: [PATCH] c++: Fix parsing [[]][[]];

[Marek Polacek]

On Tue, Dec 05, 2023 at 08:51:51AM +0100, Jakub Jelinek wrote:
> Hi!
> 
> When working on the previous patch I put [[]] [[]] asm (""); into a
> testcase, but was surprised it wasn't parsed.

By wasn't parsed you mean we gave an error, right?  I only see an error
with block-scope [[]] [[]];.

> The problem is that when cp_parser_std_attribute_spec returns NULL, it
> can mean 2 different things, one is that the next token(s) are neither
> [[ nor alignas (in that case the caller should break from the loop),
> or when we parsed something like [[]] - it was valid attribute specifier,
> but didn't specify any attributes in it.
> 
> The following patch fixes that by adding another parameter to differentiate
> between the cases, guess another option would be to use some magic
> tree value for the break case instead of NULL_TREE (but error_mark_node is
> already taken and means something else).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Or shall I go with some magic return which will never happen otherwise?
> void_node?

It seems marginally better to me to use void_list_node so that we don't
need a new parm, like what we do when parsing parameters: ()/(void)/(...),
but I should let others decide.

If we stay with any_p, please add a comment saying that it means "found any",
not "accepts any" or something else.  Thanks,

> 2023-12-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* parser.cc (cp_parser_std_attribute_spec): Add ANY_P argument, set
> 	what it points to initially to true and only if token is neither
> 	CPP_OPEN_SQUARE nor RID_ALIGNAS CPP_KEYWORD set it to false.
> 	(cp_parser_std_attribute_spec_seq): Adjust
> 	cp_parser_std_attribute_spec caller.  If it returns NULL_TREE and
> 	any_p is true, continue rather than break.
> 
> 	* g++.dg/cpp0x/gen-attrs-79.C: New test.
> 
> --- gcc/cp/parser.cc.jj	2023-12-04 20:23:53.225009856 +0100
> +++ gcc/cp/parser.cc	2023-12-04 20:49:21.160426104 +0100
> @@ -2703,7 +2703,7 @@ static tree cp_parser_gnu_attribute_list
>  static tree cp_parser_std_attribute
>    (cp_parser *, tree);
>  static tree cp_parser_std_attribute_spec
> -  (cp_parser *);
> +  (cp_parser *, bool *);
>  static tree cp_parser_std_attribute_spec_seq
>    (cp_parser *);
>  static size_t cp_parser_skip_std_attribute_spec_seq
> @@ -30265,11 +30265,12 @@ void cp_parser_late_contract_condition (
>  	 conditional-expression ] ]  */
>  
>  static tree
> -cp_parser_std_attribute_spec (cp_parser *parser)
> +cp_parser_std_attribute_spec (cp_parser *parser, bool *any_p)
>  {
>    tree attributes = NULL_TREE;
>    cp_token *token = cp_lexer_peek_token (parser->lexer);
>  
> +  *any_p = true;
>    if (token->type == CPP_OPEN_SQUARE
>        && cp_lexer_peek_nth_token (parser->lexer, 2)->type == CPP_OPEN_SQUARE)
>      {
> @@ -30342,7 +30343,10 @@ cp_parser_std_attribute_spec (cp_parser
>  
>        if (token->type != CPP_KEYWORD
>  	  || token->keyword != RID_ALIGNAS)
> -	return NULL_TREE;
> +	{
> +	  *any_p = false;
> +	  return NULL_TREE;
> +	}
>  
>        cp_lexer_consume_token (parser->lexer);
>        maybe_warn_cpp0x (CPP0X_ATTRIBUTES);
> @@ -30414,9 +30418,16 @@ cp_parser_std_attribute_spec_seq (cp_par
>  
>    while (true)
>      {
> -      tree attr_spec = cp_parser_std_attribute_spec (parser);
> +      bool any_p;
> +      tree attr_spec = cp_parser_std_attribute_spec (parser, &any_p);
>        if (attr_spec == NULL_TREE)
> -	break;
> +	{
> +	  /* Accept [[]][[]]; for which cp_parser_std_attribute_spec
> +	     also returns NULL_TREE as there are no attributes.  */
> +	  if (any_p)
> +	    continue;
> +	  break;
> +	}
>        if (attr_spec == error_mark_node)
>  	return error_mark_node;
>  
> --- gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C.jj	2023-12-04 20:38:35.122574430 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C	2023-12-04 20:38:29.468654143 +0100
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target c++11 } }
> +
> +[[]] [[]];
> +
> +[[]] [[]] void
> +foo ()
> +{
> +  [[]] [[]];
> +}
> 
> 	Jakub
> 

Marek

[PATCH] c++, v2: Fix parsing [[]][[]];

[Jakub Jelinek]

On Tue, Dec 05, 2023 at 09:45:32AM -0500, Marek Polacek wrote:
> > When working on the previous patch I put [[]] [[]] asm (""); into a
> > testcase, but was surprised it wasn't parsed.
> 
> By wasn't parsed you mean we gave an error, right?  I only see an error
> with block-scope [[]] [[]];.

Yeah.
The reason why [[]][[]]; works at namespace scope is that if
  else if (cp_lexer_nth_token_is (parser->lexer,
                                  cp_parser_skip_std_attribute_spec_seq (parser,
                                                                         1),
                                  CPP_SEMICOLON))
which is the case here then even if after parsing the attributes next token
isn't CPP_SEMICOLON (the case here without the patch), it will just return
and another cp_parser_declaration will parse another [[]], that time also
with CPP_SEMICOLON.

> It seems marginally better to me to use void_list_node so that we don't
> need a new parm, like what we do when parsing parameters: ()/(void)/(...),
> but I should let others decide.

Here is a modified version of the patch which does it like that.

2023-12-05  Jakub Jelinek  <jakub@redhat.com>

	* parser.cc (cp_parser_std_attribute_spec): Return void_list_node
	rather than NULL_TREE if token is neither CPP_OPEN_SQUARE nor
	RID_ALIGNAS CPP_KEYWORD.
	(cp_parser_std_attribute_spec_seq): For attr_spec == void_list_node
	break, for attr_spec == NULL_TREE continue.

	* g++.dg/cpp0x/gen-attrs-79.C: New test.

--- gcc/cp/parser.cc.jj	2023-12-05 16:18:32.224909370 +0100
+++ gcc/cp/parser.cc	2023-12-05 17:07:34.690170639 +0100
@@ -30244,7 +30244,11 @@ void cp_parser_late_contract_condition (
      [ [ assert :  contract-mode [opt] : conditional-expression ] ]
      [ [ pre :  contract-mode [opt] : conditional-expression ] ]
      [ [ post :  contract-mode [opt] identifier [opt] :
-	 conditional-expression ] ]  */
+	 conditional-expression ] ]
+
+   Return void_list_node if the current token doesn't start an
+   attribute-specifier to differentiate from NULL_TREE returned e.g.
+   for [ [ ] ].  */
 
 static tree
 cp_parser_std_attribute_spec (cp_parser *parser)
@@ -30324,7 +30328,7 @@ cp_parser_std_attribute_spec (cp_parser
 
       if (token->type != CPP_KEYWORD
 	  || token->keyword != RID_ALIGNAS)
-	return NULL_TREE;
+	return void_list_node;
 
       cp_lexer_consume_token (parser->lexer);
       maybe_warn_cpp0x (CPP0X_ATTRIBUTES);
@@ -30397,8 +30401,12 @@ cp_parser_std_attribute_spec_seq (cp_par
   while (true)
     {
       tree attr_spec = cp_parser_std_attribute_spec (parser);
-      if (attr_spec == NULL_TREE)
+      if (attr_spec == void_list_node)
 	break;
+      /* Accept [[]][[]]; for which cp_parser_std_attribute_spec
+	 returns NULL_TREE as there are no attributes.  */
+      if (attr_spec == NULL_TREE)
+	continue;
       if (attr_spec == error_mark_node)
 	return error_mark_node;
 
--- gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C.jj	2023-12-05 17:04:14.235988879 +0100
+++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C	2023-12-05 17:04:14.235988879 +0100
@@ -0,0 +1,9 @@
+// { dg-do compile { target c++11 } }
+
+[[]] [[]];
+
+[[]] [[]] void
+foo ()
+{
+  [[]] [[]];
+}


	Jakub

Re: [PATCH] c++, v2: Fix parsing [[]][[]];

[Marek Polacek]

On Tue, Dec 05, 2023 at 06:00:31PM +0100, Jakub Jelinek wrote:
> On Tue, Dec 05, 2023 at 09:45:32AM -0500, Marek Polacek wrote:
> > > When working on the previous patch I put [[]] [[]] asm (""); into a
> > > testcase, but was surprised it wasn't parsed.
> > 
> > By wasn't parsed you mean we gave an error, right?  I only see an error
> > with block-scope [[]] [[]];.
> 
> Yeah.
> The reason why [[]][[]]; works at namespace scope is that if
>   else if (cp_lexer_nth_token_is (parser->lexer,
>                                   cp_parser_skip_std_attribute_spec_seq (parser,
>                                                                          1),
>                                   CPP_SEMICOLON))
> which is the case here then even if after parsing the attributes next token
> isn't CPP_SEMICOLON (the case here without the patch), it will just return
> and another cp_parser_declaration will parse another [[]], that time also
> with CPP_SEMICOLON.
> 
> > It seems marginally better to me to use void_list_node so that we don't
> > need a new parm, like what we do when parsing parameters: ()/(void)/(...),
> > but I should let others decide.
> 
> Here is a modified version of the patch which does it like that.

Thanks, this looks good to me.
 
> 2023-12-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* parser.cc (cp_parser_std_attribute_spec): Return void_list_node
> 	rather than NULL_TREE if token is neither CPP_OPEN_SQUARE nor
> 	RID_ALIGNAS CPP_KEYWORD.
> 	(cp_parser_std_attribute_spec_seq): For attr_spec == void_list_node
> 	break, for attr_spec == NULL_TREE continue.
> 
> 	* g++.dg/cpp0x/gen-attrs-79.C: New test.
> 
> --- gcc/cp/parser.cc.jj	2023-12-05 16:18:32.224909370 +0100
> +++ gcc/cp/parser.cc	2023-12-05 17:07:34.690170639 +0100
> @@ -30244,7 +30244,11 @@ void cp_parser_late_contract_condition (
>       [ [ assert :  contract-mode [opt] : conditional-expression ] ]
>       [ [ pre :  contract-mode [opt] : conditional-expression ] ]
>       [ [ post :  contract-mode [opt] identifier [opt] :
> -	 conditional-expression ] ]  */
> +	 conditional-expression ] ]
> +
> +   Return void_list_node if the current token doesn't start an
> +   attribute-specifier to differentiate from NULL_TREE returned e.g.
> +   for [ [ ] ].  */
>  
>  static tree
>  cp_parser_std_attribute_spec (cp_parser *parser)
> @@ -30324,7 +30328,7 @@ cp_parser_std_attribute_spec (cp_parser
>  
>        if (token->type != CPP_KEYWORD
>  	  || token->keyword != RID_ALIGNAS)
> -	return NULL_TREE;
> +	return void_list_node;
>  
>        cp_lexer_consume_token (parser->lexer);
>        maybe_warn_cpp0x (CPP0X_ATTRIBUTES);
> @@ -30397,8 +30401,12 @@ cp_parser_std_attribute_spec_seq (cp_par
>    while (true)
>      {
>        tree attr_spec = cp_parser_std_attribute_spec (parser);
> -      if (attr_spec == NULL_TREE)
> +      if (attr_spec == void_list_node)
>  	break;
> +      /* Accept [[]][[]]; for which cp_parser_std_attribute_spec
> +	 returns NULL_TREE as there are no attributes.  */
> +      if (attr_spec == NULL_TREE)
> +	continue;
>        if (attr_spec == error_mark_node)
>  	return error_mark_node;
>  
> --- gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C.jj	2023-12-05 17:04:14.235988879 +0100
> +++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-79.C	2023-12-05 17:04:14.235988879 +0100
> @@ -0,0 +1,9 @@
> +// { dg-do compile { target c++11 } }
> +
> +[[]] [[]];
> +
> +[[]] [[]] void
> +foo ()
> +{
> +  [[]] [[]];
> +}

Marek

Re: [PATCH] c++, v2: Fix parsing [[]][[]];

[Jason Merrill]

On 12/5/23 12:17, Marek Polacek wrote:
> On Tue, Dec 05, 2023 at 06:00:31PM +0100, Jakub Jelinek wrote:
>> On Tue, Dec 05, 2023 at 09:45:32AM -0500, Marek Polacek wrote:
>>>> When working on the previous patch I put [[]] [[]] asm (""); into a
>>>> testcase, but was surprised it wasn't parsed.
>>>
>>> By wasn't parsed you mean we gave an error, right?  I only see an error
>>> with block-scope [[]] [[]];.
>>
>> Yeah.
>> The reason why [[]][[]]; works at namespace scope is that if
>>    else if (cp_lexer_nth_token_is (parser->lexer,
>>                                    cp_parser_skip_std_attribute_spec_seq (parser,
>>                                                                           1),
>>                                    CPP_SEMICOLON))
>> which is the case here then even if after parsing the attributes next token
>> isn't CPP_SEMICOLON (the case here without the patch), it will just return
>> and another cp_parser_declaration will parse another [[]], that time also
>> with CPP_SEMICOLON.
>>
>>> It seems marginally better to me to use void_list_node so that we don't
>>> need a new parm, like what we do when parsing parameters: ()/(void)/(...),
>>> but I should let others decide.
>>
>> Here is a modified version of the patch which does it like that.
> 
> Thanks, this looks good to me.

Agreed, OK.