C++ PATCH for c++/89083, c++/80864 - ICE with list initialization in template

Jason Merrill jason@redhat.com
Thu Jan 31 15:22:00 GMT 2019


On 1/31/19 9:50 AM, Marek Polacek wrote:
> On Wed, Jan 30, 2019 at 05:37:13PM -0500, Jason Merrill wrote:
>> On 1/30/19 4:15 PM, Marek Polacek wrote:
>>> On Wed, Jan 30, 2019 at 04:11:11PM -0500, Marek Polacek wrote:
>>>> On Tue, Jan 29, 2019 at 09:40:18PM -0500, Jason Merrill wrote:
>>>>> On Tue, Jan 29, 2019 at 6:53 PM Marek Polacek <polacek@redhat.com> wrote:
>>>>>>
>>>>>> My recent patch for 88815 and 78244 caused 89083, a P1 9 regression, which
>>>>>> happens to be the same problem as 80864 and its many dupes, something I'd
>>>>>> been meaning to fix for a long time.
>>>>>>
>>>>>> Basically, the problem is repeated reshaping of a constructor, once when
>>>>>> parsing, and then again when substituting.  With the recent fix, we call
>>>>>> reshape_init + digest_init in finish_compound_literal even in a template
>>>>>> if the expression is not instantiation-dependent, and then again when
>>>>>> tsubst_*.
>>>>>>
>>>>>> For instance, in initlist107.C, when parsing a functional cast, we call
>>>>>> finish_compound_literal which calls reshape_init, which turns
>>>>>>
>>>>>>     { NON_LVALUE_EXPR<1>, NON_LVALUE_EXPR<2> }
>>>>>>
>>>>>> into
>>>>>>
>>>>>>     { { NON_LVALUE_EXPR<1>, NON_LVALUE_EXPR<2> } }
>>>>>>
>>>>>> and then digest_init turns that into
>>>>>>
>>>>>>     { .x = { 1, 2 } }
>>>>>>
>>>>>> which is a compound literal (TREE_HAS_CONSTRUCTOR set), but the subexpression
>>>>>> "{ 1, 2 }" isn't.  "{ 1, 2 }" will now have the type int[3], so it's not
>>>>>> BRACE_ENCLOSED_INITIALIZER_P.
>>>>>>
>>>>>> And then tsubst_* processes "{ .x = { 1, 2 } }".  The case CONSTRUCTOR
>>>>>> in tsubst_copy_and_build will call finish_compound_literal on a copy of
>>>>>> "{ 1, 2 }" wrapped in a new { }, because the whole expr has TREE_HAS_CONSTRUCTOR.
>>>>>> That crashes in reshape_init_r in the
>>>>>>    6155       if (TREE_CODE (stripped_init) == CONSTRUCTOR)
>>>>>> block; we have a constructor, it's not COMPOUND_LITERAL_P, and because
>>>>>> digest_init had given it the type int[3], we hit
>>>>>>    6172               gcc_assert (BRACE_ENCLOSED_INITIALIZER_P (stripped_init));
>>>>>>
>>>>>> As expand_default_init explains in a comment, a CONSTRUCTOR of the target's type
>>>>>> is a previously digested initializer, so we should probably do a similar trick
>>>>>> here.  This fixes all the variants of the problem I've come up with.
>>>>>>
>>>>>> 80864 is a similar case, we reshape when parsing and then second time in
>>>>>> fold_non_dependent_expr called from store_init_value, because of the 'constexpr'.
>>>>>>
>>>>>> Also update a stale comment.
>>>>>>
>>>>>> Bootstrapped/regtest running on x86_64-linux, ok for trunk and 8 after a while?
>>>>>>
>>>>>> 2019-01-29  Marek Polacek  <polacek@redhat.com>
>>>>>>
>>>>>>           PR c++/89083, c++/80864 - ICE with list initialization in template.
>>>>>>           * decl.c (reshape_init_r): Don't reshape a digested initializer.
>>>>>>
>>>>>>           * g++.dg/cpp0x/initlist107.C: New test.
>>>>>>           * g++.dg/cpp0x/initlist108.C: New test.
>>>>>>           * g++.dg/cpp0x/initlist109.C: New test.
>>>>>>
>>>>>> diff --git gcc/cp/decl.c gcc/cp/decl.c
>>>>>> index 79eeac177b6..da08ecc21aa 100644
>>>>>> --- gcc/cp/decl.c
>>>>>> +++ gcc/cp/decl.c
>>>>>> @@ -6161,11 +6161,17 @@ reshape_init_r (tree type, reshape_iter *d, bool first_initializer_p,
>>>>>>               ;
>>>>>>             else if (COMPOUND_LITERAL_P (stripped_init))
>>>>>>             /* For a nested compound literal, there is no need to reshape since
>>>>>> -            brace elision is not allowed. Even if we decided to allow it,
>>>>>> -            we should add a call to reshape_init in finish_compound_literal,
>>>>>> -            before calling digest_init, so changing this code would still
>>>>>> -            not be necessary.  */
>>>>>> +            we called reshape_init in finish_compound_literal, before calling
>>>>>> +            digest_init.  */
>>>>>>               gcc_assert (!BRACE_ENCLOSED_INITIALIZER_P (stripped_init));
>>>>>> +         /* Similarly, a CONSTRUCTOR of the target's type is a previously
>>>>>> +            digested initializer.  */
>>>>>> +         else if (same_type_ignoring_top_level_qualifiers_p (type,
>>>>>> +                                                             TREE_TYPE (init)))
>>>>>
>>>>> Hmm, aren't both of these tests true for a dependent compound literal,
>>>>> which won't have been reshaped already?
>>>>
>>>> I'm hoping that can't happen, but it's a good question.  When we have a
>>>> dependent compound literal, finish_compound_literal just sets
>>>> TREE_HAS_CONSTRUCTOR and returns it, so then in tsubst_*, after substituting
>>>> each element of the constructor, we call finish_compound_literal.  The
>>>> constructor is no longer dependent and since we operate on a copy on which
>>>> we didn't set TREE_HAS_CONSTRUCTOR, the first condition shouldn't be true.
>>>>
>>>> And the second condition should also never be true for a compound literal
>>>> that hasn't been reshaped, because digest_init is only ever called after
>>>> reshape_init (and the comment for digest_init_r says it assumes that
>>>> reshape_init has already run).
>>
>> And because, as above, tsubst_* builds up a CONSTRUCTOR with
>> init_list_type_node and feeds that to finish_compound_literal.
> 
> Yes.
> 
>> I suppose that means we do the same thing for a non-dependent CONSTRUCTOR
>> that has already been reshaped, but it should be harmless.
>>
>>>> The type of a CONSTRUCTOR can also by changed
>>>> in tsubst_copy_and_build:
>>>> 19269         TREE_TYPE (r) = type;
>>>> but I haven't been able to trigger any problem yet.  Worst comes to worst this
>>>> patch changes the ICE to another ICE, but I'm not finding a testcase.
>>
>> I'd expect that's where the { 1, 2 } goes through to produce this issue.
> 
> Correct.  There's more to this.  When I debugged 80864 I couldn't understand
> why r216750 would make any difference here, why the type in the case
> CONSTRUCTOR was not an init list type.  It turned out that the reason was
> the new
>    
>    if (AGGREGATE_TYPE_P (type) || VECTOR_TYPE_P (type))
>      ...
> 
> block in cxx_eval_outermost_constant_expr added in r216750, because it does
> 
>        if (TREE_CODE (r) == TARGET_EXPR)
>          /* Avoid creating another CONSTRUCTOR when we expand the
>             TARGET_EXPR.  */
>          r = TARGET_EXPR_INITIAL (r);
> 
> Before r216750 we'd process the TARGET_EXPR in cxx_eval_constant_expression,
> and that has
> 
> 4422         /* Adjust the type of the result to the type of the temporary.  */
> 4423         r = adjust_temp_type (TREE_TYPE (t), r);
> 
> whereas now we extract the CONSTRUCTOR from the TARGET_EXPR, and we end up
> doing
> 
> 5176   if (TREE_CODE (r) == CONSTRUCTOR && CLASS_TYPE_P (TREE_TYPE (r)))
> 5177     {
> 5178       r = adjust_temp_type (type, r);
> 
> Now "type" here was obtained by initialized_type, which always uses
> cv_unqualified.  So while "TREE_TYPE (t)" is const something, "type" is
> without that const.  And look what adjust_temp_type does:
> 
> 1290   if (same_type_p (TREE_TYPE (temp), type))
> 1291     return temp;
> 1292   /* Avoid wrapping an aggregate value in a NOP_EXPR.  */
> 1293   if (TREE_CODE (temp) == CONSTRUCTOR)
> 1294     return build_constructor (type, CONSTRUCTOR_ELTS (temp));
> 
> So if I remember correctly in one case we returned the original ctor with
> TREE_HAS_CONSTRUCTOR preserved, and in the other case we returned a new ctor
> without TREE_HAS_CONSTRUCTOR.

> Now I still think my fix makes sense, but the above is suspicious, too.
> (Using initialized_type instead of "TREE_TYPE (t)" doesn't fix this bug.)

Hmm, that does look questionable; adjust_temp_type should probably use 
copy_node (and then change the type) rather than build_constructor. 
Does doing that also fix the bug?  Having that flag would mean 
COMPOUND_LITERAL_P is true, so adding the same_type check shouldn't be 
necessary.

>> Hmm, the PMF and nested compound literal cases above your change look like
>> they don't do what they're intended to do; they fall through to either
>> gcc_unreachable or reshape_init_*, contrary to the comments.
> 
> Things break if I return in the PMF case, we need to keep it as-is.  I've
> updated its comment though.  And I've added a new test checking "missing
> braces" for a PMF.  It matches what clang warns about.
> 
> But I merged my new case with the nested compound literal case and that
> doesn't seem to break anything.
> 
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2019-01-31  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c++/89083, c++/80864 - ICE with list initialization in template.
> 	* decl.c (reshape_init_r): Don't reshape a digested initializer.
> 	Return the initializer for COMPOUND_LITERAL_P.
> 
> 	* g++.dg/cpp0x/initlist107.C: New test.
> 	* g++.dg/cpp0x/initlist108.C: New test.
> 	* g++.dg/cpp0x/initlist109.C: New test.
> 	* g++.dg/cpp0x/initlist110.C: New test.
> 	* g++.dg/cpp0x/initlist111.C: New test.
> 	* g++.dg/cpp0x/initlist112.C: New test.
> 	* g++.dg/init/ptrfn4.C: New test.
> 
> diff --git gcc/cp/decl.c gcc/cp/decl.c
> index 79eeac177b6..de4883ff994 100644
> --- gcc/cp/decl.c
> +++ gcc/cp/decl.c
> @@ -6154,20 +6154,29 @@ reshape_init_r (tree type, reshape_iter *d, bool first_initializer_p,
>       {
>         if (TREE_CODE (stripped_init) == CONSTRUCTOR)
>   	{
> -	  if (TREE_TYPE (init) && TYPE_PTRMEMFUNC_P (TREE_TYPE (init)))
> -	    /* There is no need to reshape pointer-to-member function
> -	       initializers, as they are always constructed correctly
> -	       by the front end.  */
> -           ;
> -	  else if (COMPOUND_LITERAL_P (stripped_init))
> +	  tree init_type = TREE_TYPE (init);
> +	  if (init_type && TYPE_PTRMEMFUNC_P (init_type))
> +	    /* There is no need to call reshape_init forpointer-to-member

Missing space.

Jason



More information about the Gcc-patches mailing list