[C++] PR 78344 ("ICE on invalid c++ code (tree check: expected tree_list, have error_mark in cp_check_const_attributes, at cp/decl2.c:1347")

Hi,

in this error recovery issue cp_check_const_attributes and more 
generally cplus_decl_attributes have lots of troubles handling the 
error_mark_node returned by cp_parser_std_attribute_spec_seq, as called 
by cp_parser_direct_declarator. I fiddled quite a bit with these parsing 
facilities to eventually notice that boldly changing 
cp_parser_std_attribute_spec_seq to return NULL_TREE instead of 
error_mark_node when cp_parser_std_attribute_spec returns 
error_mark_node in the loop cures the bug at issue as a special case.

I also noticed that in cp_parser_std_attribute_spec we are using 
token_pair::require_open / require_close very peculiarly, issuing a 
cp_parser_error when the returned bool is false instead of simply 
bailing out with error_mark_node and that in fact causes duplicate 
diagnostics about expected '(' / ')', respectively.

Tested x86_64-linux.

Thanks, Paolo.

///////////////////////////
/cp
2017-12-22  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/78344
	* parser.c (cp_parser_std_attribute_spec_seq): When
	cp_parser_std_attribute_spec returns error_mark_node return
	back NULL_TREE instead of error_mark_node.

	* parser.c (cp_parser_std_attribute_spec): When
        token_pair::require_open / require_close returns false simply
	return error_mark_node, do not issue a duplicate cp_parser_error
	about expected '(' / ')', respectively.

/testsuite
2017-12-22  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/78344
	* g++.dg/cpp0x/alignas13.C: New.

On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> in this error recovery issue cp_check_const_attributes and more generally
> cplus_decl_attributes have lots of troubles handling the error_mark_node
> returned by cp_parser_std_attribute_spec_seq, as called by
> cp_parser_direct_declarator. I fiddled quite a bit with these parsing
> facilities to eventually notice that boldly changing
> cp_parser_std_attribute_spec_seq to return NULL_TREE instead of
> error_mark_node when cp_parser_std_attribute_spec returns error_mark_node in
> the loop cures the bug at issue as a special case.

Hmm, I'm skeptical.  In general, we want to use error_mark_node to
distinguish between something not being there and being there but
wrong.

> I also noticed that in cp_parser_std_attribute_spec we are using
> token_pair::require_open / require_close very peculiarly, issuing a
> cp_parser_error when the returned bool is false instead of simply bailing
> out with error_mark_node and that in fact causes duplicate diagnostics about
> expected '(' / ')', respectively.

The hunks for this issue are OK.

Jason

Hi,

On 10/01/2018 16:32, Jason Merrill wrote:
> On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> in this error recovery issue cp_check_const_attributes and more generally
>> cplus_decl_attributes have lots of troubles handling the error_mark_node
>> returned by cp_parser_std_attribute_spec_seq, as called by
>> cp_parser_direct_declarator. I fiddled quite a bit with these parsing
>> facilities to eventually notice that boldly changing
>> cp_parser_std_attribute_spec_seq to return NULL_TREE instead of
>> error_mark_node when cp_parser_std_attribute_spec returns error_mark_node in
>> the loop cures the bug at issue as a special case.
> Hmm, I'm skeptical.  In general, we want to use error_mark_node to
> distinguish between something not being there and being there but
> wrong.
I see what you mean. But consider that we already emitted diagnostic 
anyway, and, important detail, isn't that we are dropping some correct 
attributes, we are dropping all of them anyway with error_mark_node or 
NULL_TREE the same way. It's just that with NULL_TREE we are behaving 
during error recovery as if the attributes weren't there in the first 
place. I'm not sure if this was 100% clear... Would you like to have 
some additional details?

Thanks!
Paolo.

On Wed, Jan 10, 2018 at 11:33 AM, Paolo Carlini
<paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 10/01/2018 16:32, Jason Merrill wrote:
>>
>> On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini <paolo.carlini@oracle.com>
>> wrote:
>>>
>>> in this error recovery issue cp_check_const_attributes and more generally
>>> cplus_decl_attributes have lots of troubles handling the error_mark_node
>>> returned by cp_parser_std_attribute_spec_seq, as called by
>>> cp_parser_direct_declarator. I fiddled quite a bit with these parsing
>>> facilities to eventually notice that boldly changing
>>> cp_parser_std_attribute_spec_seq to return NULL_TREE instead of
>>> error_mark_node when cp_parser_std_attribute_spec returns error_mark_node
>>> in
>>> the loop cures the bug at issue as a special case.
>>
>> Hmm, I'm skeptical.  In general, we want to use error_mark_node to
>> distinguish between something not being there and being there but
>> wrong.
>
> I see what you mean. But consider that we already emitted diagnostic anyway,

I'm not sure we can rely on that; cp_parser_error doesn't emit
anything when parsing tentatively.

> and, important detail, isn't that we are dropping some correct attributes,
> we are dropping all of them anyway with error_mark_node or NULL_TREE the
> same way. It's just that with NULL_TREE we are behaving during error
> recovery as if the attributes weren't there in the first place. I'm not sure
> if this was 100% clear... Would you like to have some additional details?

It's clear, I'm just not convinced it's what we want.

Jason

Hi,

On 10/01/2018 17:58, Jason Merrill wrote:
> On Wed, Jan 10, 2018 at 11:33 AM, Paolo Carlini
> <paolo.carlini@oracle.com> wrote:
>> Hi,
>>
>> On 10/01/2018 16:32, Jason Merrill wrote:
>>> On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini <paolo.carlini@oracle.com>
>>> wrote:
>>>> in this error recovery issue cp_check_const_attributes and more generally
>>>> cplus_decl_attributes have lots of troubles handling the error_mark_node
>>>> returned by cp_parser_std_attribute_spec_seq, as called by
>>>> cp_parser_direct_declarator. I fiddled quite a bit with these parsing
>>>> facilities to eventually notice that boldly changing
>>>> cp_parser_std_attribute_spec_seq to return NULL_TREE instead of
>>>> error_mark_node when cp_parser_std_attribute_spec returns error_mark_node
>>>> in
>>>> the loop cures the bug at issue as a special case.
>>> Hmm, I'm skeptical.  In general, we want to use error_mark_node to
>>> distinguish between something not being there and being there but
>>> wrong.
>> I see what you mean. But consider that we already emitted diagnostic anyway,
> I'm not sure we can rely on that; cp_parser_error doesn't emit
> anything when parsing tentatively.
Ok. I'm going to investigate that a bit more - the obvious idea would be 
limiting somehow the approach to when we are *not* parsing tentatively - 
otherwise I'll see if we can handle elegantly enough those 
error_mark_nodes. I committed the other straightforward change which you 
approved, thanks about that.

Paolo.

Hi again,

thus the below is a rather "dull" solution at the level of 
cplus_decl_attributes itself: cp_check_const_attributes is tweaked to 
check for error_mark_node at each outer iteration and consistently 
return a bool, which is then checked by the caller in order to possibly 
bail out (this is similar to the check_for_bare_parameter_packs call a 
few lines before). Testing is Ok on x86_64-linux.

Thanks,
Paolo.

////////////////////////////
/cp
2018-01-11  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/78344
	* decl2.c (cp_check_const_attributes): Check for error_mark_node
	at each outer iterations; return a bool.
	(cplus_decl_attributes): Adjust cp_check_const_attributes call.

/testsuite
2018-01-10  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/78344
	* g++.dg/cpp0x/alignas13.C: New.
Index: cp/decl2.c
===================================================================
--- cp/decl2.c	(revision 256451)
+++ cp/decl2.c	(working copy)
@@ -1396,19 +1396,17 @@ cp_reconstruct_complex_type (tree type, tree botto
 }
 
 /* Replaces any constexpr expression that may be into the attributes
-   arguments with their reduced value.  */
+   arguments with their reduced value.  Returns FALSE if encounters
+   error_mark_node, TRUE otherwise.  */
 
-static void
+static bool
 cp_check_const_attributes (tree attributes)
 {
-  if (attributes == error_mark_node)
-    return;
-
-  tree attr;
-  for (attr = attributes; attr; attr = TREE_CHAIN (attr))
+  for (tree attr = attributes; attr; attr = TREE_CHAIN (attr))
     {
-      tree arg;
-      for (arg = TREE_VALUE (attr); arg; arg = TREE_CHAIN (arg))
+      if (attr == error_mark_node)
+	return false;
+      for (tree arg = TREE_VALUE (attr); arg; arg = TREE_CHAIN (arg))
 	{
 	  tree expr = TREE_VALUE (arg);
 	  if (EXPR_P (expr))
@@ -1415,6 +1413,7 @@ cp_check_const_attributes (tree attributes)
 	    TREE_VALUE (arg) = maybe_constant_value (expr);
 	}
     }
+  return true;
 }
 
 /* Return true if TYPE is an OpenMP mappable type.  */
@@ -1528,7 +1527,8 @@ cplus_decl_attributes (tree *decl, tree attributes
       save_template_attributes (&attributes, decl, flags);
     }
 
-  cp_check_const_attributes (attributes);
+  if (!cp_check_const_attributes (attributes))
+    return;
 
   if (TREE_CODE (*decl) == TEMPLATE_DECL)
     decl = &DECL_TEMPLATE_RESULT (*decl);
Index: testsuite/g++.dg/cpp0x/alignas13.C
===================================================================
--- testsuite/g++.dg/cpp0x/alignas13.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/alignas13.C	(working copy)
@@ -0,0 +1,5 @@
+// PR c++/78344
+// { dg-do compile { target c++11 } }
+
+alignas(double) int f alignas;         // { dg-error "30:expected '\\('" }
+alignas(double) int g alignas(double;  // { dg-error "37:expected '\\)'" }

... today I played a bit with the other idea inspired by your feedback. 
Irrespective of the special issue at hand, it seems in principle 
interesting to me that when we are sure that we aren't parsing 
tentatively anymore we can do something more radical for the sake of 
better error recovery. Anyway, the below passes testing and yes, there 
are cases in our testsuite where cp_parser_std_attribute_spec returns 
error_mark_node and cp_parser_uncommitted_to_tentative_parse_p is true.

Paolo.

///////////////////
Index: cp/parser.c
===================================================================
--- cp/parser.c	(revision 256543)
+++ cp/parser.c	(working copy)
@@ -25382,7 +25382,15 @@ cp_parser_std_attribute_spec_seq (cp_parser *parse
       if (attr_spec == NULL_TREE)
 	break;
       if (attr_spec == error_mark_node)
-	return error_mark_node;
+	{
+	  if (!cp_parser_uncommitted_to_tentative_parse_p (parser))
+	    /* When guaranteed to be correct, behaving as if the
+	       erroneous attributes weren't there in the first place
+	       makes for very good error recovery (c++/78344).  */
+	    return NULL_TREE;
+	  else
+	    return error_mark_node;
+	}
 
       if (attr_last)
 	TREE_CHAIN (attr_last) = attr_spec;
Index: testsuite/g++.dg/cpp0x/alignas13.C
===================================================================
--- testsuite/g++.dg/cpp0x/alignas13.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/alignas13.C	(working copy)
@@ -0,0 +1,5 @@
+// PR c++/78344
+// { dg-do compile { target c++11 } }
+
+alignas(double) int f alignas;         // { dg-error "30:expected '\\('" }
+alignas(double) int g alignas(double;  // { dg-error "37:expected '\\)'" }

On 01/10/2018 06:50 PM, Paolo Carlini wrote:
> thus the below is a rather "dull" solution at the level of 
> cplus_decl_attributes itself: cp_check_const_attributes is tweaked to 
> check for error_mark_node at each outer iteration

This shouldn't be necessary; we should have returned error_mark_node for 
the attribute list rather than append it to something else, in which 
case the existing check for attributes == error_mark_node would have 
done the job.

Jason

Hi,

On 11/01/2018 21:33, Jason Merrill wrote:
> On 01/10/2018 06:50 PM, Paolo Carlini wrote:
>> thus the below is a rather "dull" solution at the level of 
>> cplus_decl_attributes itself: cp_check_const_attributes is tweaked to 
>> check for error_mark_node at each outer iteration
>
> This shouldn't be necessary; we should have returned error_mark_node 
> for the attribute list rather than append it to something else, in 
> which case the existing check for attributes == error_mark_node would 
> have done the job.
Indeed. That seems the most straightforward way to approach the issue. 
Thanks.

In grokdeclarator we could either explicitly assign error_mark_node to 
*attrlist (instead of chaining) or simply drop the erroneous 
declarator->std_attributes. Both solutions appear to work fine in 
practice, pass testing.

Paolo.

/////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 256556)
+++ cp/decl.c	(working copy)
@@ -11487,9 +11487,15 @@ grokdeclarator (const cp_declarator *declarator,
       && declarator->kind == cdk_id
       && declarator->std_attributes
       && attrlist != NULL)
-    /* [dcl.meaning]/1: The optional attribute-specifier-seq following
-       a declarator-id appertains to the entity that is declared.  */
-    *attrlist = chainon (*attrlist, declarator->std_attributes);
+    {
+      /* [dcl.meaning]/1: The optional attribute-specifier-seq following
+	 a declarator-id appertains to the entity that is declared.  */
+      if (declarator->std_attributes != error_mark_node)
+	*attrlist = chainon (*attrlist, declarator->std_attributes);
+      else
+	/* We should have already diagnosed the issue (c++/78344).  */
+	*attrlist = error_mark_node;
+    }
 
   /* Handle parameter packs. */
   if (parameter_pack_p)
Index: testsuite/g++.dg/cpp0x/alignas13.C
===================================================================
--- testsuite/g++.dg/cpp0x/alignas13.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/alignas13.C	(working copy)
@@ -0,0 +1,5 @@
+// PR c++/78344
+// { dg-do compile { target c++11 } }
+
+alignas(double) int f alignas;         // { dg-error "30:expected '\\('" }
+alignas(double) int g alignas(double;  // { dg-error "37:expected '\\)'" }
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 256556)
+++ cp/decl.c	(working copy)
@@ -11486,6 +11486,8 @@ grokdeclarator (const cp_declarator *declarator,
   if (declarator
       && declarator->kind == cdk_id
       && declarator->std_attributes
+      /* We should have already diagnosed the issue (c++/78344).  */
+      && declarator->std_attributes != error_mark_node
       && attrlist != NULL)
     /* [dcl.meaning]/1: The optional attribute-specifier-seq following
        a declarator-id appertains to the entity that is declared.  */
Index: testsuite/g++.dg/cpp0x/alignas13.C
===================================================================
--- testsuite/g++.dg/cpp0x/alignas13.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/alignas13.C	(working copy)
@@ -0,0 +1,5 @@
+// PR c++/78344
+// { dg-do compile { target c++11 } }
+
+alignas(double) int f alignas;         // { dg-error "30:expected '\\('" }
+alignas(double) int g alignas(double;  // { dg-error "37:expected '\\)'" }

On Thu, Jan 11, 2018 at 5:11 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> On 11/01/2018 21:33, Jason Merrill wrote:
>> On 01/10/2018 06:50 PM, Paolo Carlini wrote:
>>>
>>> thus the below is a rather "dull" solution at the level of
>>> cplus_decl_attributes itself: cp_check_const_attributes is tweaked to check
>>> for error_mark_node at each outer iteration
>>
>> This shouldn't be necessary; we should have returned error_mark_node for
>> the attribute list rather than append it to something else, in which case
>> the existing check for attributes == error_mark_node would have done the
>> job.
>
> Indeed. That seems the most straightforward way to approach the issue.
> Thanks.
>
> In grokdeclarator we could either explicitly assign error_mark_node to
> *attrlist (instead of chaining) or simply drop the erroneous
> declarator->std_attributes. Both solutions appear to work fine in practice,
> pass testing.

Hmm, I think dropping the attributes is reasonable for grokdeclarator
to do as error-recovery, similarly to how it discards an ill-formed
exception-specification.  But let's assert seen_error() in that case.

Jason

Hi,

On 12/01/2018 19:13, Jason Merrill wrote:
> Hmm, I think dropping the attributes is reasonable for grokdeclarator
> to do as error-recovery, similarly to how it discards an ill-formed
> exception-specification.  But let's assert seen_error() in that case.
Agreed. The below passes testing.

Thanks!
Paolo.

///////////////////////
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 256592)
+++ cp/decl.c	(working copy)
@@ -11487,9 +11487,15 @@ grokdeclarator (const cp_declarator *declarator,
       && declarator->kind == cdk_id
       && declarator->std_attributes
       && attrlist != NULL)
-    /* [dcl.meaning]/1: The optional attribute-specifier-seq following
-       a declarator-id appertains to the entity that is declared.  */
-    *attrlist = chainon (*attrlist, declarator->std_attributes);
+    {
+      /* [dcl.meaning]/1: The optional attribute-specifier-seq following
+	 a declarator-id appertains to the entity that is declared.  */
+      if (declarator->std_attributes != error_mark_node)
+	*attrlist = chainon (*attrlist, declarator->std_attributes);
+      else
+	/* We should have already diagnosed the issue (c++/78344).  */
+	gcc_assert (seen_error ());
+    }
 
   /* Handle parameter packs. */
   if (parameter_pack_p)
Index: testsuite/g++.dg/cpp0x/alignas13.C
===================================================================
--- testsuite/g++.dg/cpp0x/alignas13.C	(nonexistent)
+++ testsuite/g++.dg/cpp0x/alignas13.C	(working copy)
@@ -0,0 +1,5 @@
+// PR c++/78344
+// { dg-do compile { target c++11 } }
+
+alignas(double) int f alignas;         // { dg-error "30:expected '\\('" }
+alignas(double) int g alignas(double;  // { dg-error "37:expected '\\)'" }

On Fri, Jan 12, 2018 at 01:13:17PM -0500, Jason Merrill wrote:
> On Thu, Jan 11, 2018 at 5:11 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> > On 11/01/2018 21:33, Jason Merrill wrote:
> >> On 01/10/2018 06:50 PM, Paolo Carlini wrote:
> >>>
> >>> thus the below is a rather "dull" solution at the level of
> >>> cplus_decl_attributes itself: cp_check_const_attributes is tweaked to check
> >>> for error_mark_node at each outer iteration
> >>
> >> This shouldn't be necessary; we should have returned error_mark_node for
> >> the attribute list rather than append it to something else, in which case
> >> the existing check for attributes == error_mark_node would have done the
> >> job.
> >
> > Indeed. That seems the most straightforward way to approach the issue.
> > Thanks.
> >
> > In grokdeclarator we could either explicitly assign error_mark_node to
> > *attrlist (instead of chaining) or simply drop the erroneous
> > declarator->std_attributes. Both solutions appear to work fine in practice,
> > pass testing.
> 
> Hmm, I think dropping the attributes is reasonable for grokdeclarator
> to do as error-recovery, similarly to how it discards an ill-formed
> exception-specification.  But let's assert seen_error() in that case.

PR83824 is simiar to this PR, just in a different spot, but again:
13406		    decl_specs->attributes
13407		      = chainon (decl_specs->attributes,
13408				 attrs);
in parser.c and decl_specs->attributes is error_mark_node and attrs
is error_mark_node too, yet seen_error () is false.  The reason for that
is that we are doing tentative parsing here, so e.g.
      if (!parens.require_close (parser))
        return error_mark_node;
doesn't report any error but just returns error_mark_node.
So, either we want to go with what Paolo posted even in this case,
i.e. turn decl_specs->attributes into error_mark_node if attrs
is error_mark_node, and don't chainon anything to it if decl_specs->attributes
is already error_mark_node, e.g. something like:
if (attrs == error_mark_node || decl_specs->attributes == error_mark_node)
  decl_specs->attributes = error_mark_node;
else
  decl_specs->attributes = chainon (decl_specs->attributes, attrs);
or we can not add error_mark_node attributes at all, but we can't assert
seen_error () in that case; perhaps track in a side boolean in decl_specs
whether attributes is errorneous.

	Jakub

On Sat, Jan 13, 2018 at 12:10:22PM +0100, Jakub Jelinek wrote:
> On Fri, Jan 12, 2018 at 01:13:17PM -0500, Jason Merrill wrote:
> > On Thu, Jan 11, 2018 at 5:11 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> > > On 11/01/2018 21:33, Jason Merrill wrote:
> > >> On 01/10/2018 06:50 PM, Paolo Carlini wrote:
> > >>>
> > >>> thus the below is a rather "dull" solution at the level of
> > >>> cplus_decl_attributes itself: cp_check_const_attributes is tweaked to check
> > >>> for error_mark_node at each outer iteration
> > >>
> > >> This shouldn't be necessary; we should have returned error_mark_node for
> > >> the attribute list rather than append it to something else, in which case
> > >> the existing check for attributes == error_mark_node would have done the
> > >> job.
> > >
> > > Indeed. That seems the most straightforward way to approach the issue.
> > > Thanks.
> > >
> > > In grokdeclarator we could either explicitly assign error_mark_node to
> > > *attrlist (instead of chaining) or simply drop the erroneous
> > > declarator->std_attributes. Both solutions appear to work fine in practice,
> > > pass testing.
> > 
> > Hmm, I think dropping the attributes is reasonable for grokdeclarator
> > to do as error-recovery, similarly to how it discards an ill-formed
> > exception-specification.  But let's assert seen_error() in that case.
> 
> PR83824 is simiar to this PR, just in a different spot, but again:
> 13406		    decl_specs->attributes
> 13407		      = chainon (decl_specs->attributes,
> 13408				 attrs);
> in parser.c and decl_specs->attributes is error_mark_node and attrs
> is error_mark_node too, yet seen_error () is false.  The reason for that
> is that we are doing tentative parsing here, so e.g.
>       if (!parens.require_close (parser))
>         return error_mark_node;
> doesn't report any error but just returns error_mark_node.
> So, either we want to go with what Paolo posted even in this case,
> i.e. turn decl_specs->attributes into error_mark_node if attrs
> is error_mark_node, and don't chainon anything to it if decl_specs->attributes
> is already error_mark_node, e.g. something like:
> if (attrs == error_mark_node || decl_specs->attributes == error_mark_node)
>   decl_specs->attributes = error_mark_node;
> else
>   decl_specs->attributes = chainon (decl_specs->attributes, attrs);
> or we can not add error_mark_node attributes at all, but we can't assert
> seen_error () in that case; perhaps track in a side boolean in decl_specs
> whether attributes is errorneous.

Or we could not add those error_mark_nodes and
  gcc_assert (seen_error () || cp_parser_error_occurred (parser));

	Jakub

On Sat, Jan 13, 2018 at 12:12:02PM +0100, Jakub Jelinek wrote:
> Or we could not add those error_mark_nodes and
>   gcc_assert (seen_error () || cp_parser_error_occurred (parser));

This fixes the testcase:
--- gcc/cp/parser.c.jj	2018-01-11 18:58:48.386391801 +0100
+++ gcc/cp/parser.c	2018-01-13 12:17:20.545347195 +0100
@@ -13403,6 +13403,9 @@ cp_parser_decl_specifier_seq (cp_parser*
 		}
 	    }
 
+	  if (attrs == error_mark_node)
+	    gcc_assert (seen_error () || cp_parser_error_occurred (parser));
+	  else
 	    decl_specs->attributes
 	      = chainon (decl_specs->attributes,
 			 attrs);
but there are 13 chainon calls like this in parser.c.  Shouldn't we introduce
a helper function for this, perhaps:

void
attr_chainon (cp_parser *parser, tree *attrs, tree attr)
{
  /* Don't add error_mark_node to the chain, it can't be chained.
     Assert this is during error recovery.  */
  if (attr == error_mark_node)
    gcc_assert (seen_error () || cp_parser_error_occurred (parser));
  else
    *attrs = chainon (*attrs, attr);
}

and change all affected spots, like

	  attr_chainon (parser, &decl_specs->attributes, attrs);

?

	Jakub

Hi Jakub, all,

> On 13 Jan 2018, at 12:32, Jakub Jelinek <jakub@redhat.com> wrote:
> 
>> On Sat, Jan 13, 2018 at 12:12:02PM +0100, Jakub Jelinek wrote:
>> Or we could not add those error_mark_nodes and
>>  gcc_assert (seen_error () || cp_parser_error_occurred (parser));
> 
> This fixes the testcase:
> --- gcc/cp/parser.c.jj    2018-01-11 18:58:48.386391801 +0100
> +++ gcc/cp/parser.c    2018-01-13 12:17:20.545347195 +0100
> @@ -13403,6 +13403,9 @@ cp_parser_decl_specifier_seq (cp_parser*
>        }
>        }
> 
> +      if (attrs == error_mark_node)
> +        gcc_assert (seen_error () || cp_parser_error_occurred (parser));
> +      else
>        decl_specs->attributes
>          = chainon (decl_specs->attributes,
>             attrs);
> but there are 13 chainon calls like this in parser.c.  Shouldn't we introduce
> a helper function for this, perhaps:
> 
> void
> attr_chainon (cp_parser *parser, tree *attrs, tree attr)
> {
>  /* Don't add error_mark_node to the chain, it can't be chained.
>     Assert this is during error recovery.  */
>  if (attr == error_mark_node)
>    gcc_assert (seen_error () || cp_parser_error_occurred (parser));
>  else
>    *attrs = chainon (*attrs, attr);
> }
> 
> and change all affected spots, like
> 
>      attr_chainon (parser, &decl_specs->attributes, attrs);
> 
> ?

First, thanks for your messages. Personally, at this late time for 8, I vote for something like my most recent grokdeclarator fix and yours above for 83824. Then, for 9, or even 8.2, the more encompassing change for all those chainons. Please both of you let me know how shall we proceed, I could certainly take care of the latter too from now on. Thanks again!

Paolo

On Sat, Jan 13, 2018 at 7:59 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi Jakub, all,
>
>> On 13 Jan 2018, at 12:32, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>>> On Sat, Jan 13, 2018 at 12:12:02PM +0100, Jakub Jelinek wrote:
>>> Or we could not add those error_mark_nodes and
>>>  gcc_assert (seen_error () || cp_parser_error_occurred (parser));
>>
>> This fixes the testcase:
>> --- gcc/cp/parser.c.jj    2018-01-11 18:58:48.386391801 +0100
>> +++ gcc/cp/parser.c    2018-01-13 12:17:20.545347195 +0100
>> @@ -13403,6 +13403,9 @@ cp_parser_decl_specifier_seq (cp_parser*
>>        }
>>        }
>>
>> +      if (attrs == error_mark_node)
>> +        gcc_assert (seen_error () || cp_parser_error_occurred (parser));
>> +      else
>>        decl_specs->attributes
>>          = chainon (decl_specs->attributes,
>>             attrs);
>> but there are 13 chainon calls like this in parser.c.  Shouldn't we introduce
>> a helper function for this, perhaps:
>>
>> void
>> attr_chainon (cp_parser *parser, tree *attrs, tree attr)
>> {
>>  /* Don't add error_mark_node to the chain, it can't be chained.
>>     Assert this is during error recovery.  */
>>  if (attr == error_mark_node)
>>    gcc_assert (seen_error () || cp_parser_error_occurred (parser));
>>  else
>>    *attrs = chainon (*attrs, attr);
>> }
>>
>> and change all affected spots, like
>>
>>      attr_chainon (parser, &decl_specs->attributes, attrs);
>>
>> ?
>
> First, thanks for your messages. Personally, at this late time for 8, I vote for something like my most recent grokdeclarator fix and yours above for 83824. Then, for 9, or even 8.2, the more encompassing change for all those chainons. Please both of you let me know how shall we proceed, I could certainly take care of the latter too from now on. Thanks again!

Let's go ahead with your patch to grokdeclarator.  In the parser,
let's do what Jakub is suggesting here:

> So, either we want to go with what Paolo posted even in this case,
> i.e. turn decl_specs->attributes into error_mark_node if attrs
> is error_mark_node, and don't chainon anything to it if decl_specs->attributes
> is already error_mark_node, e.g. something like:
> if (attrs == error_mark_node || decl_specs->attributes == error_mark_node)
>   decl_specs->attributes = error_mark_node;
> else
>   decl_specs->attributes = chainon (decl_specs->attributes, attrs);

without any assert.  Putting this logic in an attr_chainon function sounds good.

Jason

On Wed, Jan 17, 2018 at 02:31:29PM -0500, Jason Merrill wrote:
> > First, thanks for your messages. Personally, at this late time for 8, I vote for something like my most recent grokdeclarator fix and yours above for 83824. Then, for 9, or even 8.2, the more encompassing change for all those chainons. Please both of you let me know how shall we proceed, I could certainly take care of the latter too from now on. Thanks again!
> 
> Let's go ahead with your patch to grokdeclarator.  In the parser,
> let's do what Jakub is suggesting here:
> 
> > So, either we want to go with what Paolo posted even in this case,
> > i.e. turn decl_specs->attributes into error_mark_node if attrs
> > is error_mark_node, and don't chainon anything to it if decl_specs->attributes
> > is already error_mark_node, e.g. something like:
> > if (attrs == error_mark_node || decl_specs->attributes == error_mark_node)
> >   decl_specs->attributes = error_mark_node;
> > else
> >   decl_specs->attributes = chainon (decl_specs->attributes, attrs);
> 
> without any assert.  Putting this logic in an attr_chainon function sounds good.

So like this?  So far just tested with make check-c++-all on both
x86_64-linux and i686-linux, full bootstrap/regtest scheduled, ok if it
passes?  I gave up on the original idea to return void and have the first
argument pointer, because while many of the calls do x = chainon (x, y);,
there are several ones that assign it to something else, like y = chainon (x, y);
etc.

2018-01-17  Jakub Jelinek  <jakub@redhat.com>

	PR c++/83824
	* parser.c (attr_chainon): New function.
	(cp_parser_label_for_labeled_statement, cp_parser_decl_specifier_seq,
	cp_parser_namespace_definition, cp_parser_init_declarator,
	cp_parser_type_specifier_seq, cp_parser_parameter_declaration,
	cp_parser_gnu_attributes_opt): Use it.
	(cp_parser_member_declaration, cp_parser_objc_class_ivars,
	cp_parser_objc_struct_declaration): Likewise.  Don't reset
	prefix_attributes if attributes is error_mark_node.

	* g++.dg/cpp0x/pr83824.C: New test.

--- gcc/cp/parser.c.jj	2018-01-13 17:57:38.115836072 +0100
+++ gcc/cp/parser.c	2018-01-17 20:46:21.809738257 +0100
@@ -10908,6 +10908,18 @@ cp_parser_statement (cp_parser* parser,
 		"attributes at the beginning of statement are ignored");
 }
 
+/* Append ATTR to attribute list ATTRS.  */
+
+static tree
+attr_chainon (tree attrs, tree attr)
+{
+  if (attrs == error_mark_node)
+    return error_mark_node;
+  if (attr == error_mark_node)
+    return error_mark_node;
+  return chainon (attrs, attr);
+}
+
 /* Parse the label for a labeled-statement, i.e.
 
    identifier :
@@ -11027,7 +11039,7 @@ cp_parser_label_for_labeled_statement (c
       else if (!cp_parser_parse_definitely (parser))
 	;
       else
-	attributes = chainon (attributes, attrs);
+	attributes = attr_chainon (attributes, attrs);
     }
 
   if (attributes != NULL_TREE)
@@ -13394,8 +13406,7 @@ cp_parser_decl_specifier_seq (cp_parser*
 		  else
 		    {
 		      decl_specs->std_attributes
-			= chainon (decl_specs->std_attributes,
-				   attrs);
+			= attr_chainon (decl_specs->std_attributes, attrs);
 		      if (decl_specs->locations[ds_std_attribute] == 0)
 			decl_specs->locations[ds_std_attribute] = token->location;
 		    }
@@ -13403,9 +13414,8 @@ cp_parser_decl_specifier_seq (cp_parser*
 		}
 	    }
 
-	    decl_specs->attributes
-	      = chainon (decl_specs->attributes,
-			 attrs);
+	  decl_specs->attributes
+	    = attr_chainon (decl_specs->attributes, attrs);
 	  if (decl_specs->locations[ds_attribute] == 0)
 	    decl_specs->locations[ds_attribute] = token->location;
 	  continue;
@@ -18471,7 +18481,7 @@ cp_parser_namespace_definition (cp_parse
 	  identifier = cp_parser_identifier (parser);
 
 	  /* Parse any attributes specified after the identifier.  */
-	  attribs = chainon (attribs, cp_parser_attributes_opt (parser));
+	  attribs = attr_chainon (attribs, cp_parser_attributes_opt (parser));
 	}
 
       if (cp_lexer_next_token_is_not (parser->lexer, CPP_SCOPE))
@@ -19633,7 +19643,7 @@ cp_parser_init_declarator (cp_parser* pa
       decl = grokfield (declarator, decl_specifiers,
 			initializer, !is_non_constant_init,
 			/*asmspec=*/NULL_TREE,
-			chainon (attributes, prefix_attributes));
+			attr_chainon (attributes, prefix_attributes));
       if (decl && TREE_CODE (decl) == FUNCTION_DECL)
 	cp_parser_save_default_args (parser, decl);
       cp_finalize_omp_declare_simd (parser, decl);
@@ -21007,9 +21017,9 @@ cp_parser_type_specifier_seq (cp_parser*
       /* Check for attributes first.  */
       if (cp_next_tokens_can_be_attribute_p (parser))
 	{
-	  type_specifier_seq->attributes =
-	    chainon (type_specifier_seq->attributes,
-		     cp_parser_attributes_opt (parser));
+	  type_specifier_seq->attributes
+	    = attr_chainon (type_specifier_seq->attributes,
+			    cp_parser_attributes_opt (parser));
 	  continue;
 	}
 
@@ -21491,8 +21501,8 @@ cp_parser_parameter_declaration (cp_pars
       parser->default_arg_ok_p = saved_default_arg_ok_p;
       /* After the declarator, allow more attributes.  */
       decl_specifiers.attributes
-	= chainon (decl_specifiers.attributes,
-		   cp_parser_attributes_opt (parser));
+	= attr_chainon (decl_specifiers.attributes,
+			cp_parser_attributes_opt (parser));
 
       /* If the declarator is a template parameter pack, remember that and
 	 clear the flag in the declarator itself so we don't get errors
@@ -23653,13 +23663,13 @@ cp_parser_member_declaration (cp_parser*
 		  late_attributes = cp_parser_attributes_opt (parser);
 		}
 
-	      attributes = chainon (attributes, late_attributes);
+	      attributes = attr_chainon (attributes, late_attributes);
 
 	      /* Remember which attributes are prefix attributes and
 		 which are not.  */
 	      first_attribute = attributes;
 	      /* Combine the attributes.  */
-	      attributes = chainon (prefix_attributes, attributes);
+	      attributes = attr_chainon (prefix_attributes, attributes);
 
 	      /* Create the bitfield declaration.  */
 	      decl = grokbitfield (identifier
@@ -23715,7 +23725,7 @@ cp_parser_member_declaration (cp_parser*
 		 which are not.  */
 	      first_attribute = attributes;
 	      /* Combine the attributes.  */
-	      attributes = chainon (prefix_attributes, attributes);
+	      attributes = attr_chainon (prefix_attributes, attributes);
 
 	      /* If it's an `=', then we have a constant-initializer or a
 		 pure-specifier.  It is not correct to parse the
@@ -23837,10 +23847,13 @@ cp_parser_member_declaration (cp_parser*
 	  cp_finalize_oacc_routine (parser, decl, false);
 
 	  /* Reset PREFIX_ATTRIBUTES.  */
-	  while (attributes && TREE_CHAIN (attributes) != first_attribute)
-	    attributes = TREE_CHAIN (attributes);
-	  if (attributes)
-	    TREE_CHAIN (attributes) = NULL_TREE;
+	  if (attributes != error_mark_node)
+	    {
+	      while (attributes && TREE_CHAIN (attributes) != first_attribute)
+		attributes = TREE_CHAIN (attributes);
+	      if (attributes)
+		TREE_CHAIN (attributes) = NULL_TREE;
+	    }
 
 	  /* If there is any qualification still in effect, clear it
 	     now; we will be starting fresh with the next declarator.  */
@@ -24909,7 +24922,7 @@ cp_parser_gnu_attributes_opt (cp_parser*
 	cp_parser_skip_to_end_of_statement (parser);
 
       /* Add these new attributes to the list.  */
-      attributes = chainon (attributes, attribute_list);
+      attributes = attr_chainon (attributes, attribute_list);
     }
 
   return attributes;
@@ -30114,7 +30127,7 @@ cp_parser_objc_class_ivars (cp_parser* p
 	     which are not.  */
 	  first_attribute = attributes;
 	  /* Combine the attributes.  */
-	  attributes = chainon (prefix_attributes, attributes);
+	  attributes = attr_chainon (prefix_attributes, attributes);
 
 	  if (width)
 	    /* Create the bitfield declaration.  */
@@ -30130,10 +30143,13 @@ cp_parser_objc_class_ivars (cp_parser* p
 	    objc_add_instance_variable (decl);
 
 	  /* Reset PREFIX_ATTRIBUTES.  */
-	  while (attributes && TREE_CHAIN (attributes) != first_attribute)
-	    attributes = TREE_CHAIN (attributes);
-	  if (attributes)
-	    TREE_CHAIN (attributes) = NULL_TREE;
+	  if (attributes != error_mark_node)
+	    {
+	      while (attributes && TREE_CHAIN (attributes) != first_attribute)
+		attributes = TREE_CHAIN (attributes);
+	      if (attributes)
+		TREE_CHAIN (attributes) = NULL_TREE;
+	    }
 
 	  token = cp_lexer_peek_token (parser->lexer);
 
@@ -30666,8 +30682,8 @@ cp_parser_objc_struct_declaration (cp_pa
 	 which are not.  */
       first_attribute = attributes;
       /* Combine the attributes.  */
-      attributes = chainon (prefix_attributes, attributes);
-      
+      attributes = attr_chainon (prefix_attributes, attributes);
+
       decl = grokfield (declarator, &declspecs,
 			NULL_TREE, /*init_const_expr_p=*/false,
 			NULL_TREE, attributes);
@@ -30676,10 +30692,13 @@ cp_parser_objc_struct_declaration (cp_pa
 	return error_mark_node;
       
       /* Reset PREFIX_ATTRIBUTES.  */
-      while (attributes && TREE_CHAIN (attributes) != first_attribute)
-	attributes = TREE_CHAIN (attributes);
-      if (attributes)
-	TREE_CHAIN (attributes) = NULL_TREE;
+      if (attributes != error_mark_node)
+	{
+	  while (attributes && TREE_CHAIN (attributes) != first_attribute)
+	    attributes = TREE_CHAIN (attributes);
+	  if (attributes)
+	    TREE_CHAIN (attributes) = NULL_TREE;
+	}
 
       DECL_CHAIN (decl) = decls;
       decls = decl;
--- gcc/testsuite/g++.dg/cpp0x/pr83824.C.jj	2018-01-17 20:52:24.224696009 +0100
+++ gcc/testsuite/g++.dg/cpp0x/pr83824.C	2018-01-17 20:52:03.091698473 +0100
@@ -0,0 +1,9 @@
+// PR c++/83824
+// { dg-do compile { target c++11 } }
+
+void
+foo ()
+{
+  if (alignas(1 alignas(1)))	// { dg-error "expected" }
+    ;
+}


	Jakub

On Wed, Jan 17, 2018 at 4:07 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 17, 2018 at 02:31:29PM -0500, Jason Merrill wrote:
>> > First, thanks for your messages. Personally, at this late time for 8, I vote for something like my most recent grokdeclarator fix and yours above for 83824. Then, for 9, or even 8.2, the more encompassing change for all those chainons. Please both of you let me know how shall we proceed, I could certainly take care of the latter too from now on. Thanks again!
>>
>> Let's go ahead with your patch to grokdeclarator.  In the parser,
>> let's do what Jakub is suggesting here:
>>
>> > So, either we want to go with what Paolo posted even in this case,
>> > i.e. turn decl_specs->attributes into error_mark_node if attrs
>> > is error_mark_node, and don't chainon anything to it if decl_specs->attributes
>> > is already error_mark_node, e.g. something like:
>> > if (attrs == error_mark_node || decl_specs->attributes == error_mark_node)
>> >   decl_specs->attributes = error_mark_node;
>> > else
>> >   decl_specs->attributes = chainon (decl_specs->attributes, attrs);
>>
>> without any assert.  Putting this logic in an attr_chainon function sounds good.
>
> So like this?  So far just tested with make check-c++-all on both
> x86_64-linux and i686-linux, full bootstrap/regtest scheduled, ok if it
> passes?  I gave up on the original idea to return void and have the first
> argument pointer, because while many of the calls do x = chainon (x, y);,
> there are several ones that assign it to something else, like y = chainon (x, y);
> etc.

Looks good.

Jason