On Thu, Jan 31, 2019 at 10:17:54AM -0500, Jason Merrill wrote:
> 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 <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.
> >
> > 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.
Ok, so that doesn't help at all -- I guess because we now use initialized_type
without cv-quals, the same_type_p check applies and adjust_temp_type returns
the original expression, so it's not the case that we're losing the
TREE_HAS_CONSTRUCTOR bit here.
For initlist107.C we don't call adjust_temp_type at all.
> > > 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 <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.
> > 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.
Fixed.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2019-01-31 Marek Polacek <pola...@redhat.com>
PR c++/89083, c++/80864 - ICE with list initialization in template.
* constexpr.c (adjust_temp_type): Use copy_node and change the type
instead of using build_constructor.
* 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/constexpr.c gcc/cp/constexpr.c
index 42681416760..19eb44fc0c0 100644
--- gcc/cp/constexpr.c
+++ gcc/cp/constexpr.c
@@ -1291,7 +1291,12 @@ adjust_temp_type (tree type, tree temp)
return temp;
/* Avoid wrapping an aggregate value in a NOP_EXPR. */
if (TREE_CODE (temp) == CONSTRUCTOR)
- return build_constructor (type, CONSTRUCTOR_ELTS (temp));
+ {
+ /* build_constructor wouldn't retain various CONSTRUCTOR flags. */
+ tree t = copy_node (temp);
+ TREE_TYPE (t) = type;
+ return t;
+ }
if (TREE_CODE (temp) == EMPTY_CLASS_EXPR)
return build0 (EMPTY_CLASS_EXPR, type);
gcc_assert (scalarish_type_p (type));
diff --git gcc/cp/decl.c gcc/cp/decl.c
index 79eeac177b6..65ba812deb6 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 for pointer-to-member
+ function initializers, as they are always constructed correctly
+ by the front end. Here we have e.g. {.__pfn=0B, .__delta=0},
+ which is missing outermost braces. We should warn below, and
+ one of the routines below will wrap it in additional { }. */;
/* 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. */
- gcc_assert (!BRACE_ENCLOSED_INITIALIZER_P (stripped_init));
+ we called reshape_init in finish_compound_literal, before calling
+ digest_init. */
+ else if (COMPOUND_LITERAL_P (stripped_init)
+ /* Similarly, a CONSTRUCTOR of the target's type is a
+ previously digested initializer. */
+ || same_type_ignoring_top_level_qualifiers_p (type,
+ init_type))
+ {
+ ++d->cur;
+ gcc_assert (!BRACE_ENCLOSED_INITIALIZER_P (stripped_init));
+ return init;
+ }
else
{
+ /* Something that hasn't been reshaped yet. */
++d->cur;
gcc_assert (BRACE_ENCLOSED_INITIALIZER_P (stripped_init));
return reshape_init (type, init, complain);
diff --git gcc/testsuite/g++.dg/cpp0x/initlist107.C
gcc/testsuite/g++.dg/cpp0x/initlist107.C
new file mode 100644
index 00000000000..9acfe7cb267
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist107.C
@@ -0,0 +1,24 @@
+// PR c++/89083
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wmissing-braces" }
+
+struct A { int x[3]; };
+
+template<class T>
+decltype(A{1, 2}, T()) fn1(T t) // { dg-warning "missing braces" }
+{
+ return t;
+}
+
+template<class T>
+decltype(A{{1, 2}}, T()) fn2(T t)
+{
+ return t;
+}
+
+void
+f()
+{
+ fn1(1);
+ fn2(1);
+}
diff --git gcc/testsuite/g++.dg/cpp0x/initlist108.C
gcc/testsuite/g++.dg/cpp0x/initlist108.C
new file mode 100644
index 00000000000..e2839787fa5
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist108.C
@@ -0,0 +1,34 @@
+// PR c++/80864
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wmissing-braces" }
+
+struct S {
+ char c[1];
+};
+
+template <typename T>
+void
+fn ()
+{
+ constexpr S s1 = S{};
+ constexpr S s2 = S{{}};
+ constexpr S s3 = S{{{}}};
+ constexpr S s4 = {};
+ constexpr S s5 = {{}};
+ constexpr S s6 = {{{}}};
+ constexpr S s7{{}};
+ constexpr S s8{S{}};
+ constexpr S s9{S{{}}};
+ constexpr S s10{S{{{}}}};
+ constexpr S s11 = S();
+ constexpr S s12 = S({});
+ constexpr S s13 = S({{}});
+ constexpr S s14 = {{}};
+ constexpr S s15 = {{{}}};
+}
+
+void
+foo ()
+{
+ fn<int>();
+}
diff --git gcc/testsuite/g++.dg/cpp0x/initlist109.C
gcc/testsuite/g++.dg/cpp0x/initlist109.C
new file mode 100644
index 00000000000..1351a2d57ce
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist109.C
@@ -0,0 +1,15 @@
+// PR c++/80864
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wmissing-braces" }
+
+struct S {};
+struct A { S s[1]; };
+
+template <typename>
+struct R { static constexpr auto h = A{S{}}; }; // { dg-warning "missing
braces" }
+
+template <typename>
+struct R2 { static constexpr auto h = A{{S{}}}; };
+
+A foo = R<int>::h;
+A foo2 = R2<int>::h;
diff --git gcc/testsuite/g++.dg/cpp0x/initlist110.C
gcc/testsuite/g++.dg/cpp0x/initlist110.C
new file mode 100644
index 00000000000..7bb229cbc7e
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist110.C
@@ -0,0 +1,32 @@
+// PR c++/89083
+// { dg-do compile { target c++11 } }
+
+struct C { int a[3]; int i; };
+struct B { C c[3]; };
+struct A { B b[3]; };
+
+template<class T, int N>
+decltype(A{N, N}, T()) fn1(T t)
+{
+ return t;
+}
+
+template<class T, int N>
+decltype(A{{{N, N, N}, {N + 1}}}, T()) fn2(T t)
+{
+ return t;
+}
+
+template<class T, int N, int M>
+decltype(A{{N + M}}, T()) fn3(T t)
+{
+ return t;
+}
+
+void
+f()
+{
+ fn1<int, 10>(1);
+ fn2<int, 10>(1);
+ fn3<int, 10, 20>(1);
+}
diff --git gcc/testsuite/g++.dg/cpp0x/initlist111.C
gcc/testsuite/g++.dg/cpp0x/initlist111.C
new file mode 100644
index 00000000000..7f96115e618
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist111.C
@@ -0,0 +1,32 @@
+// PR c++/80864
+// { dg-do compile { target c++11 } }
+
+struct S {
+ int c[3];
+};
+
+template <typename T, int N>
+void
+fn ()
+{
+ constexpr S s1 = S{N};
+ constexpr S s2 = S{{N, N}};
+ constexpr S s3 = S{N, N};
+ constexpr S s4 = {N};
+ constexpr S s5 = {{N}};
+ constexpr S s6 = {N, N};
+ constexpr S s7{{N}};
+ constexpr S s8{S{N}};
+ constexpr S s9{S{{N}}};
+ constexpr S s10{S{{N}}};
+ constexpr S s11 = S({N});
+ constexpr S s12 = S({{N}});
+ constexpr S s13 = {{N}};
+ constexpr S s14 = {{N, N, N}};
+}
+
+void
+foo ()
+{
+ fn<int, 10>();
+}
diff --git gcc/testsuite/g++.dg/cpp0x/initlist112.C
gcc/testsuite/g++.dg/cpp0x/initlist112.C
new file mode 100644
index 00000000000..cd97098eec5
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/initlist112.C
@@ -0,0 +1,14 @@
+// PR c++/80864
+// { dg-do compile { target c++11 } }
+
+struct S {int a[2]; };
+struct A { S s[1]; };
+
+template <typename, int N>
+struct R { static constexpr auto h = A{S{N}}; };
+
+template <typename, int N>
+struct R2 { static constexpr auto h = A{S{{N, N}}}; };
+
+A foo = R<int, 10>::h;
+A foo2 = R2<int, 10>::h;
diff --git gcc/testsuite/g++.dg/init/ptrfn4.C gcc/testsuite/g++.dg/init/ptrfn4.C
new file mode 100644
index 00000000000..e1635c86483
--- /dev/null
+++ gcc/testsuite/g++.dg/init/ptrfn4.C
@@ -0,0 +1,19 @@
+// { dg-do compile }
+// { dg-options "-Wmissing-braces" }
+
+struct S { };
+typedef void (S::*fptr1) (int);
+
+struct A {
+ fptr1 f;
+};
+
+A a[] =
+{
+ (fptr1) 0,
+}; // { dg-warning "missing braces around initializer" }
+
+A a2[] =
+{
+ { (fptr1) 0 }
+};
