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

[Jason Merrill]

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 <pola...@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  <pola...@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.

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.

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.

Jason