On 4/27/22 16:30, Marek Polacek wrote:
On Wed, Apr 27, 2022 at 11:00:46AM -0400, Patrick Palka wrote:
On Tue, 26 Apr 2022, Marek Polacek wrote:
Consider
struct A {
int x;
int y = x;
};
struct B {
int x = 0;
int y = A{x}.y; // #1
};
where for #1 we end up with
{.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
that is, two PLACEHOLDER_EXPRs for different types on the same level in
a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to
avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it.
Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing
cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use
finish_compound_literal
on type=A, compound_literal={((struct B *) this)->x}. When digesting this
initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we
don't
have any object to refer to yet. After digesting, we have
{.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor
CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns
TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct
A>)->x}>
Then we get to
B b = {};
and call store_init_value, which digest the {}, which produces
{.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x,
.y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y}
The call to replace_placeholders in store_init_value will not do anything:
we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only
a sub-expression, so replace_placeholders does nothing, so the <P_E struct B>
stays even though now is the perfect time to replace it because we have an
object for it: 'b'.
Later, in cp_gimplify_init_expr the *expr_p is
D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct
A>)->x}
where D.2395 is of type A, but we crash because we hit <P_E struct B>, which
has a different type.
My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the
TARGET_EXPR because that means we have an object we can refer to. Then clear
CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR
in the {}. Then store_init_value will be able to replace <P_E struct B> with
'b', and we should be good to go.
Makes sense to me. It seems all was well until break_out_target_exprs,
called from get_nsdmi for B::y, replaced the 'this' in the initializer
(TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct
A>)->x}>).y;
with a PLACEHOLDER_EXPR;
(TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR
struct A>)->x}>).y;
This seems to be the wrong thing to do when the 'this' appears inside a
CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new
PLACEHOLDER_EXPR then can't be resolved correctly.
Exactly.
So in light of this I wonder if we should instead perform this handling
you added to finish_compound_literal in break_out_target_exprs /
bot_manip instead?
Unfortunately that causes an ICE in gimplify_var_or_parm_decl on the new
testcase I've added here. bot_manip is a different context and so I can't
use parsing_nsdmi anymore, and it seems we'd replace the placeholders too
aggressively in bot_manip. So I'm not sure if that's the best place.
-- >8 --
Consider
struct A {
int x;
int y = x;
};
struct B {
int x = 0;
int y = A{x}.y; // #1
};
where for #1 we end up with
{.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
that is, two PLACEHOLDER_EXPRs for different types on the same level in
a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to
avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it.
Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing
cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use
finish_compound_literal
on type=A, compound_literal={((struct B *) this)->x}. When digesting this
initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we
don't
have any object to refer to yet. After digesting, we have
{.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor
CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns
TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct
A>)->x}>
Then we get to
B b = {};
and call store_init_value, which digest the {}, which produces
{.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x,
.y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y}
The call to replace_placeholders in store_init_value will not do anything:
we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only
a sub-expression, so replace_placeholders does nothing, so the <P_E struct B>
stays even though now is the perfect time to replace it because we have an
object for it: 'b'.
Later, in cp_gimplify_init_expr the *expr_p is
D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct
A>)->x}
where D.2395 is of type A, but we crash because we hit <P_E struct B>, which
has a different type.
My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the
TARGET_EXPR because that means we have an object we can refer to. Then clear
CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR
in the {}. Then store_init_value will be able to replace <P_E struct B> with
'b', and we should be good to go.
PR c++/100252
gcc/cp/ChangeLog:
* semantics.cc (finish_compound_literal): replace_placeholders after
creating the TARGET_EXPR.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/nsdmi-aggr14.C: New test.
* g++.dg/cpp1y/nsdmi-aggr15.C: New test.
---
gcc/cp/semantics.cc | 31 +++++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++++
3 files changed, 106 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index ab48f11c9be..770369458bb 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree
compound_literal,
if (TREE_CODE (compound_literal) == CONSTRUCTOR)
TREE_HAS_CONSTRUCTOR (compound_literal) = false;
compound_literal = get_target_expr_sfinae (compound_literal, complain);
+ /* We may have A{} in a NSDMI. */
+ if (parsing_nsdmi ())
+ {
+ /* Digesting the {} could have introduced a PLACEHOLDER_EXPR
+ referring to A. Now that we've built up a TARGET_EXPR, we
+ have an object we can refer to. The reason we bother doing
+ this here is for code like
+
+ struct A {
+ int x;
+ int y = x;
+ };
+
+ struct B {
+ int x = 0;
+ int y = A{x}.y; // #1
+ };
+
+ where in #1 we don't want to end up with two PLACEHOLDER_EXPRs
+ for different types on the same level in a {} as in 100252. */
+ tree init = TARGET_EXPR_INITIAL (compound_literal);
+ if (TREE_CODE (init) == CONSTRUCTOR
+ && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init))
+ {
+ tree obj = TARGET_EXPR_SLOT (compound_literal);
+ replace_placeholders (compound_literal, obj);
+ /* We should have dealt with the PLACEHOLDER_EXPRs. */
+ CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false;
+ gcc_checking_assert (!find_placeholders (init));
Does this testcase still work with this patch?
struct A {
const A* p = this;
};
struct B {
A a = A{};
};
constexpr B b;
static_assert (b.a.p == &b.a);
+ }
+ }
}
else
/* For e.g. int{42} just make sure it's a prvalue. */
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
new file mode 100644
index 00000000000..7d508f52b48
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
@@ -0,0 +1,46 @@
+// PR c++/100252
+// { dg-do run { target c++14 } }
+
+#define SA(X) static_assert ((X),#X)
+
+struct A {
+ int x;
+ int y = x;
+};
+
+struct B {
+ int x = 0;
+ int y = A{x}.y;
+};
+
+constexpr B csb1 = { };
+SA(csb1.x == 0 && csb1.y == csb1.x);
+constexpr B csb2 = { 1 };
+SA(csb2.x == 1 && csb2.y == csb2.x);
+constexpr B csb3 = { 1, 2 };
+SA(csb3.x == 1 && csb3.y == 2);
+
+B sb1 = { };
+B sb2 = { 1 };
+B sb3 = { 1, 2};
+
+int
+main ()
+{
+ if (sb1.x != 0 || sb1.x != sb1.y)
+ __builtin_abort();
+ if (sb2.x != 1 || sb2.x != sb2.y)
+ __builtin_abort();
+ if (sb3.x != 1 || sb3.y != 2)
+ __builtin_abort();
+
+ B b1 = { };
+ B b2 = { 1 };
+ B b3 = { 1, 2};
+ if (b1.x != 0 || b1.x != b1.y)
+ __builtin_abort();
+ if (b2.x != 1 || b2.x != b2.y)
+ __builtin_abort();
+ if (b3.x != 1 || b3.y != 2)
+ __builtin_abort();
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
new file mode 100644
index 00000000000..bc997bb5e1d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
@@ -0,0 +1,29 @@
+// PR c++/100252
+// { dg-do run { target c++14 } }
+
+struct A {
+ int x;
+ int y = x;
+};
+
+struct B {
+ int x = 0;
+ int y = A{x}.y;
+};
+
+void
+g (B b1 = B{}, B b2 = B{1}, B b3 = B{1, 2})
+{
+ if (b1.x != 0 || b1.y != b1.x)
+ __builtin_abort();
+ if (b2.x != 1 || b2.y != b2.x)
+ __builtin_abort();
+ if (b3.x != 1 || b3.y != 2)
+ __builtin_abort();
+}
+
+int
+main ()
+{
+ g ();
+}
base-commit: 409edcca331296b53842c50d3b789e1b1ccc05e5
