C++ PATCH for c++/89083, c++/80864 - ICE with list initialization in template
Marek Polacek
polacek@redhat.com
Thu Jan 31 14:54:00 GMT 2019
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, 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
+ 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 }
+};
More information about the Gcc-patches
mailing list