On 5/7/22 15:11, Marek Polacek wrote:
On Tue, May 03, 2022 at 04:59:38PM -0400, Jason Merrill wrote:
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);
Ouch, no. Thanks for catching this, it would have been very unpleasant
to hit this problem later...
The reason your testcase was rejected is that replacing a PLACEHOLDER
in the outermost TARGET_EXPR broke guaranteed copy elision. Now that
I've spent another day on this, I still think we have to replace the
placeholders created when temporary materialization takes place in an
NSDMI, but it must happen in digest_nsdmi_init (as Patrick suggested),
where we can see the full initializer. That way I can avoid breaking
copy elision.
I've added a bunch of new testcases and updated the
(already long) description. The yuge comment hopefully also explains
how this problem is avoided. Thanks again,
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >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 digests 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}
lookup_placeholder in constexpr won't find an object to replace the
PLACEHOLDER_EXPR for B, because ctx->object will be D.2395 of type A, and we
cannot search outward from D.2395 to find 'b'.
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 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. We must
be careful not to break guaranteed copy elision, so this replacement
happens in digest_nsdmi_init where we can see the whole initializer,
and avoid replacing any placeholders in the outermost TARGET_EXPR.
PR c++/100252
gcc/cp/ChangeLog:
* typeck2.cc (replace_placeholders_for_class_temp_r): New.
(digest_nsdmi_init): Call it.
gcc/testsuite/ChangeLog:
* g++.dg/cpp1y/nsdmi-aggr14.C: New test.
* g++.dg/cpp1y/nsdmi-aggr15.C: New test.
* g++.dg/cpp1y/nsdmi-aggr16.C: New test.
* g++.dg/cpp1y/nsdmi-aggr17.C: New test.
* g++.dg/cpp1y/nsdmi-aggr18.C: New test.
---
gcc/cp/typeck2.cc | 54 +++++++++++++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C | 29 ++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C | 19 ++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C | 47 ++++++++++++++++++++
gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C | 25 +++++++++++
6 files changed, 220 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr15.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C
diff --git a/gcc/cp/typeck2.cc b/gcc/cp/typeck2.cc
index 63d95c1529a..72d7cfaf1d3 100644
--- a/gcc/cp/typeck2.cc
+++ b/gcc/cp/typeck2.cc
@@ -1371,6 +1371,34 @@ digest_init_flags (tree type, tree init, int flags,
tsubst_flags_t complain)
return digest_init_r (type, init, 0, flags, complain);
}
+/* Callback to replace PLACEHOLDER_EXPRs in a TARGET_EXPR (which isn't used
+ in the context of guaranteed copy elision). */
+
+static tree
+replace_placeholders_for_class_temp_r (tree *tp, int *walk_subtrees, void
*data)
+{
+ tree t = *tp;
+ tree full_expr = *static_cast<tree *>(data);
+
+ /* We're looking for a TARGET_EXPR nested in the whole expression. */
+ if (TREE_CODE (t) == TARGET_EXPR && t != full_expr)
Just comparing to full_expr doesn't seem enough to catch all the cases
where a prvalue is used as an initializer rather than for temporary
materialization; you will also need to handle comma and conditional
expressions. And probably PAREN_EXPR.
I'm failing to find a term for this in the standard; "potential result"
also includes objects that the result is an xvalue subobject of (as in
#1 below). Maybe "potential prvalue result", to exclude the xvalue
cases? So,
&& !potential_prvalue_result_of (t, full_expr)
?
+ {
+ tree init = TARGET_EXPR_INITIAL (t);
+ if (TREE_CODE (init) == CONSTRUCTOR
+ && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init))
+ {
+ tree obj = TARGET_EXPR_SLOT (t);
+ replace_placeholders (t, obj);
+ /* We should have dealt with all PLACEHOLDER_EXPRs. */
+ CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false;
+ gcc_checking_assert (!find_placeholders (init));
+ }
+ *walk_subtrees = false;
+ }
+
+ return NULL_TREE;
+}
+
/* Process the initializer INIT for an NSDMI DECL (a FIELD_DECL). */
tree
digest_nsdmi_init (tree decl, tree init, tsubst_flags_t complain)
@@ -1390,6 +1418,32 @@ digest_nsdmi_init (tree decl, tree init, tsubst_flags_t
complain)
&& CP_AGGREGATE_TYPE_P (type))
init = reshape_init (type, init, complain);
init = digest_init_flags (type, init, flags, complain);
+
+ /* We may have temporary materialization in a NSDMI, if the initializer
+ has something like A{} in it. Digesting the {} could have introduced
+ a PLACEHOLDER_EXPR referring to A. Now that we've got 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 {} when lookup_placeholder
+ wouldn't find a named object for the PLACEHOLDER_EXPR for A. Note,
+ temporary materialization does not occur when initializing an object
+ from a prvalue of the same type, therefore we must not replace the
+ placeholder with a temporary object, so that it can be elided. */
+ cp_walk_tree (&init, replace_placeholders_for_class_temp_r, &init,
+ nullptr);
+
return init;
}
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 ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C
b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C
new file mode 100644
index 00000000000..a4d7428c1e7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr16.C
@@ -0,0 +1,19 @@
+// PR c++/100252
+// { dg-do compile { target c++14 } }
+
+struct A {
+ const A* p = this;
+};
+
+struct B {
+ A a = A{};
+};
+
+constexpr B b;
+static_assert (b.a.p == &b.a, "");
+B b1 = { };
+
+void
+g (B b2 = B{})
+{
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C
b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C
new file mode 100644
index 00000000000..c3bfcfa572d
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr17.C
@@ -0,0 +1,47 @@
+// PR c++/100252
+// { dg-do run { target c++14 } }
+
+struct A {
+ int x;
+ int y = x;
+ const A* p = this;
+};
+
+struct B {
+ int x = 42;
+ A a = A{x};
+};
+
+constexpr B b;
+static_assert (b.a.p == &b.a, "");
+static_assert (b.x == 42, "");
+B b2 = { };
+B b3 = { 42 };
+
+struct C {
+ int x = 42;
+ B b = B{x};
+};
+
+constexpr C c;
+C c2;
+C c3;
+
+void
+g (B b4 = B{}, B b5 = B{ 42 }, C c4 = C{}, C c5 = C{ 42 })
+{
+ if (b2.x != 42 || b2.a.x != 42 || b2.a.y != b2.a.x)
+ __builtin_abort ();
+ if (b3.x != 42 || b3.a.x != 42 || b3.a.y != b3.a.x)
+ __builtin_abort ();
+ if (b4.x != 42 || b4.a.x != 42 || b4.a.y != b4.a.x)
+ __builtin_abort ();
+ if (b5.x != 42 || b5.a.x != 42 || b5.a.y != b5.a.x)
+ __builtin_abort ();
+}
+
+int
+main ()
+{
+ g ();
+}
diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C
b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C
new file mode 100644
index 00000000000..65e2275353b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr18.C
@@ -0,0 +1,25 @@
+// PR c++/100252
+// { dg-do compile { target c++14 } }
+
+struct B { };
+
+struct A {
+ int x;
+ int y = x;
+ constexpr operator B() { return B{}; }
+};
+
+struct C {
+ int x = 42;
+ B b = A{x};
+};
+
+C c1 = {};
+C c2 = { 42 };
+constexpr C c3 = {};
+constexpr C c4 = { 42 };
+
+void
+g (C c5 = C{}, C c6 = C{ 42 })
+{
+}
base-commit: 0c723bb4be2a67657828b692997855afcdc5d286
