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.
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.
Jason
