Re: [C++ Patch] 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")

2018-01-17 Thread Jason Merrill 
On Wed, Jan 17, 2018 at 4:07 PM, Jakub Jelinek  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

Re: [C++ Patch] 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")

2018-01-17 Thread Jakub Jelinek 
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  

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*

Re: [C++ Patch] 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")

2018-01-17 Thread Jason Merrill 
On Sat, Jan 13, 2018 at 7:59 AM, Paolo Carlini  wrote:
> Hi Jakub, all,
>
>> On 13 Jan 2018, at 12:32, Jakub Jelinek  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.jj2018-01-11 18:58:48.386391801 +0100
>> +++ gcc/cp/parser.c2018-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, _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

Re: [C++ Patch] 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")

2018-01-13 Thread Paolo Carlini 
Hi Jakub, all,

> On 13 Jan 2018, at 12:32, Jakub Jelinek  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.jj2018-01-11 18:58:48.386391801 +0100
> +++ gcc/cp/parser.c2018-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, _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

Re: [C++ Patch] 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")

2018-01-13 Thread Jakub Jelinek 
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, _specs->attributes, attrs);

?

Jakub

Re: [C++ Patch] 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")

2018-01-13 Thread Jakub Jelinek 
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  
> > 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

Re: [C++ Patch] 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")

2018-01-13 Thread Jakub Jelinek 
On Fri, Jan 12, 2018 at 01:13:17PM -0500, Jason Merrill wrote:
> On Thu, Jan 11, 2018 at 5:11 PM, Paolo Carlini  
> 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,
13408attrs);
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

Re: [C++ Patch] 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")

2018-01-12 Thread Paolo Carlini 

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 '\\)'" }

Re: [C++ Patch] 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")

2018-01-12 Thread Jason Merrill 
On Thu, Jan 11, 2018 at 5:11 PM, Paolo Carlini  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

Re: [C++ Patch] 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")

2018-01-11 Thread Paolo Carlini 

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 '\\)'" }

Re: [C++ Patch] 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")

2018-01-11 Thread Jason Merrill 

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

Re: [C++ Patch] 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")

2018-01-11 Thread Paolo Carlini 
... 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 '\\)'" }

Re: [C++ Patch] 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")

2018-01-10 Thread Paolo Carlini 

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  

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  

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 (, decl, flags);
 }
 
-  cp_check_const_attributes (attributes);
+  if (!cp_check_const_attributes (attributes))
+return;
 
   if (TREE_CODE (*decl) == TEMPLATE_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 '\\)'" }

Re: [C++ Patch] 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")

2018-01-10 Thread Paolo Carlini 

Hi,

On 10/01/2018 17:58, Jason Merrill wrote:

On Wed, Jan 10, 2018 at 11:33 AM, Paolo Carlini
 wrote:

Hi,

On 10/01/2018 16:32, Jason Merrill wrote:

On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini 
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.

Re: [C++ Patch] 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")

2018-01-10 Thread Jason Merrill 
On Wed, Jan 10, 2018 at 11:33 AM, Paolo Carlini
 wrote:
> Hi,
>
> On 10/01/2018 16:32, Jason Merrill wrote:
>>
>> On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini 
>> 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

Re: [C++ Patch] 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")

2018-01-10 Thread Paolo Carlini 

Hi,

On 10/01/2018 16:32, Jason Merrill wrote:

On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini  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.

Re: [C++ Patch] 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")

2018-01-10 Thread Jason Merrill 
On Fri, Dec 22, 2017 at 7:34 PM, Paolo Carlini  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

[C++ Patch] 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")

2017-12-22 Thread Paolo Carlini 

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  

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  

PR c++/78344
* g++.dg/cpp0x/alignas13.C: New.
Index: cp/parser.c
===
--- cp/parser.c (revision 255983)
+++ cp/parser.c (working copy)
@@ -25300,10 +25300,7 @@ cp_parser_std_attribute_spec (cp_parser *parser)
 
   matching_parens parens;
   if (!parens.require_open (parser))
-   {
- cp_parser_error (parser, "expected %<(%>");
- return error_mark_node;
-   }
+   return error_mark_node;
 
   cp_parser_parse_tentatively (parser);
   alignas_expr = cp_parser_type_id (parser);
@@ -25333,10 +25330,7 @@ cp_parser_std_attribute_spec (cp_parser *parser)
return error_mark_node;
 
   if (!parens.require_close (parser))
-   {
- cp_parser_error (parser, "expected %<)%>");
- return error_mark_node;
-   }
+   return error_mark_node;
 
   /* Build the C++-11 representation of an 'aligned'
 attribute.  */
@@ -25367,7 +25361,7 @@ 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;
+   return NULL_TREE;
 
   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 '\\)'" }