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