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")

[Jason Merrill <jason at redhat dot com>]

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