[PATCH] c++: Stray RESULT_DECLs in result of constexpr function call [PR94034]
Patrick Palka
ppalka@redhat.com
Sun Apr 12 13:43:52 GMT 2020
On Sat, 11 Apr 2020, Jason Merrill wrote:
> On 4/10/20 5:47 PM, Patrick Palka wrote:
> > On Fri, 10 Apr 2020, Jason Merrill wrote:
> > > On 4/10/20 2:15 PM, Patrick Palka wrote:
> > > > On Fri, 10 Apr 2020, Patrick Palka wrote:
> > > >
> > > > > On Fri, 10 Apr 2020, Jason Merrill wrote:
> > > > >
> > > > > > On 4/10/20 1:04 PM, Patrick Palka wrote:
> > > > > > > On Thu, 9 Apr 2020, Patrick Palka wrote:
> > > > > > > > On Thu, 9 Apr 2020, Jason Merrill wrote:
> > > > > > > >
> > > > > > > > > On 4/8/20 7:49 PM, Patrick Palka wrote:
> > > > > > > > > > When evaluating the initializer of 'a' in the following
> > > > > > > > > > example
> > > > > > > > > >
> > > > > > > > > > struct A { A *p = this; };
> > > > > > > > > > constexpr A foo() { return {}; }
> > > > > > > > > > constexpr A a = foo();
> > > > > > > > > >
> > > > > > > > > > the PLACEHOLDER_EXPR for 'this' in the aggregate initializer
> > > > > > > > > > returned
> > > > > > > > > > by foo
> > > > > > > > > > gets resolved to the RESULT_DECL of foo. But due to
> > > > > > > > > > guaranteed
> > > > > > > > > > RVO,
> > > > > > > > > > the
> > > > > > > > > > 'this'
> > > > > > > > > > should really be resolved to '&a'.
> > > > > > > > >
> > > > > > > > > > It seems to me that the right approach would be to
> > > > > > > > > > immediately
> > > > > > > > > > resolve
> > > > > > > > > > the
> > > > > > > > > > PLACEHOLDER_EXPR to the correct target object during
> > > > > > > > > > evaluation
> > > > > > > > > > of
> > > > > > > > > > 'foo()',
> > > > > > > > > > so
> > > > > > > > > > that we could use 'this' to access objects adjacent to the
> > > > > > > > > > current
> > > > > > > > > > object in
> > > > > > > > > > the
> > > > > > > > > > ultimate storage location. (I think #c2 of PR c++/94537 is
> > > > > > > > > > an
> > > > > > > > > > example
> > > > > > > > > > of
> > > > > > > > > > such
> > > > > > > > > > usage of 'this', which currently doesn't work. But as #c1
> > > > > > > > > > shows
> > > > > > > > > > we
> > > > > > > > > > don't
> > > > > > > > > > seem
> > > > > > > > > > to handle this case correctly in non-constexpr
> > > > > > > > > > initialization
> > > > > > > > > > either.)
> > > > > > > > >
> > > > > > > > > As I commented in the PR, the standard doesn't require this to
> > > > > > > > > work
> > > > > > > > > because A
> > > > > > > > > is trivially copyable, and our ABI makes it impossible. But
> > > > > > > > > there's
> > > > > > > > > still a
> > > > > > > > > constexpr bug when we add
> > > > > > > > >
> > > > > > > > > A() = default; A(const A&);
> > > > > > > > >
> > > > > > > > > clang doesn't require the constructors to make this work for
> > > > > > > > > constant
> > > > > > > > > initialization, but similarly can't make it work for
> > > > > > > > > non-constant
> > > > > > > > > initialization.
> > > > > > > >
> > > > > > > > That makes a lot of sense, thanks for the detailed explanation.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > I haven't yet been able to make a solution using the above
> > > > > > > > > > approach
> > > > > > > > > > work --
> > > > > > > > > > making sure we use the ultimate object instead of the
> > > > > > > > > > RESULT_DECL
> > > > > > > > > > whenever
> > > > > > > > > > we
> > > > > > > > > > access ctx->global->values is proving to be tricky and
> > > > > > > > > > subtle.
> > > > > > > > >
> > > > > > > > > Do we need to go through ctx->global->values? Would it work
> > > > > > > > > for
> > > > > > > > > the
> > > > > > > > > RESULT_DECL case in cxx_eval_constant_expression to go to
> > > > > > > > > straight
> > > > > > > > > to
> > > > > > > > > ctx->object or ctx->ctor instead?
> > > > > > > >
> > > > > > > > I attempted that at some point, but IIRC we still end up not
> > > > > > > > resolving
> > > > > > > > some RESULT_DECLs because not all of them get processed through
> > > > > > > > cxx_eval_constant_expression before we manipulate
> > > > > > > > ctx->global->values
> > > > > > > > with them. I'll try this approach more carefully and report
> > > > > > > > back
> > > > > > > > with
> > > > > > > > more specifics.
> > > > > > >
> > > > > > > It turns out that immediately resolving RESULT_DECLs/'this' to the
> > > > > > > ultimate ctx->object would not interact well with constexpr_call
> > > > > > > caching:
> > > > > > >
> > > > > > > struct A { A() = default; A(const A&); A *ap = this; };
> > > > > > > constexpr A foo() { return {}; }
> > > > > > > constexpr A a = foo();
> > > > > > > constexpr A b = foo();
> > > > > > > static_assert(b.ap == &b); // fails
> > > > > > >
> > > > > > > Evaluation of the first call to foo() returns {&a}, since we
> > > > > > > resolve
> > > > > > > 'this' to &a due to guaranteed RVO, and we cache this result.
> > > > > > > Evaluation of the second call to foo() just returns the cached
> > > > > > > result
> > > > > > > from the constexpr_call cache, and so we get {&a} again.
> > > > > > >
> > > > > > > So it seems we would have to mark the result of a constexpr call
> > > > > > > as
> > > > > > > not
> > > > > > > cacheable whenever this RVO applies during its evaluation, even
> > > > > > > when
> > > > > > > doing the RVO has no observable difference in the final result
> > > > > > > (i.e.
> > > > > > > the
> > > > > > > constructor does not try to save the 'this' pointer).
> > > > > > >
> > > > > > > Would the performance impact of disabling caching whenever RVO
> > > > > > > applies
> > > > > > > during constexpr call evaluation be worth it, or should we go with
> > > > > > > something like my first patch which "almost works," and which
> > > > > > > marks a
> > > > > > > constexpr call as not cacheable only when we replace a RESULT_DECL
> > > > > > > in
> > > > > > > the result of the call?
> > > > > >
> > > > > > Could we search through the result of the call for ctx->object and
> > > > > > cache
> > > > > > if we
> > > > > > don't find it?
> > > > >
> > > > > Hmm, I think the result of the call could still depend on ctx->object
> > > > > without ctx->object explicitly appearing in the result. Consider the
> > > > > following testcase:
> > > > >
> > > > > struct A {
> > > > > A() = default; A(const A&);
> > > > > constexpr A(const A *q) : d{this - p} { }
> > > >
> > > > Oops sorry, that 'q' should be a 'p'.
> > > >
> > > > > long d = 0;
> > > > > };
> > > > >
> > > > > constexpr A baz(const A *q) { return A(p); };
> > > >
> > > > And same here.
> > > >
> > > > > constexpr A a = baz(&a);
> > > > > constexpr A b = baz(&a); // no error
> > > > >
> > > > > The initialization of 'b' should be ill-formed, but the result of the
> > > > > first call to baz(&a) would be {0}, so we would cache it and then
> > > > > reuse
> > > > > the result when initializing 'b'.
> > >
> > > Ah, true. Can we still cache if we're initializing something that isn't
> > > TREE_STATIC?
> >
> > Hmm, we correctly compile the analogous non-TREE_STATIC testcase
> >
> > struct A {
> > A() = default; A(const A&);
> > constexpr A(const A *p) : d{this == p} { }
> > bool d;
> > };
> >
> > constexpr A baz(const A *p) { return A(p); }
> >
> > void foo() {
> > constexpr A x = baz(&x);
> > constexpr A y = baz(&x);
> > static_assert(!y.d);
> > }
> >
> > because the calls 'baz(&x)' are considered to have non-constant arguments.
> >
> >
> > Unfortunately, we can come up with another trick to fool the constexpr_call
> > cache in the presence of RVO even in the !TREE_STATIC case:
> >
> > struct B {
> > B() = default; B(const B&);
> > constexpr B(int) : q{!this[-1].q} { }
> > bool q = false;
> > };
> >
> > constexpr B baz() { return B(0); }
> >
> > void foo()
> > {
> > constexpr B d[2] = { {}, baz() };
> > constexpr B e = baz();
> > }
> >
> > The initialization of the local variable 'e' should be invalid, but if we
> > cached the first call to 'baz' then we would wrongly accept it.
>
> Right.
>
> We ought to be able to distinguish between uses of the RESULT_DECL only for
> storing to it (cacheable) and any other use, either reading from it or messing
> with its address. But I think that's a future direction, and we should stick
> with your patch that almost works for GCC 10.
Sounds good. Does the following then look OK to commit?
-- >8 --
Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034]
When evaluating the initializer of 'a' in the following example
struct A {
A() = default; A(const A&);
A *p = this;
};
constexpr A foo() { return {}; }
constexpr A a = foo();
the PLACEHOLDER_EXPR for 'this' in the aggregate initializer returned by foo
gets resolved to the RESULT_DECL of foo. But due to guaranteed RVO, the 'this'
should really be resolved to '&a'.
Fixing this properly by immediately resolving 'this' and PLACEHOLDER_EXPRs to
the ultimate object under construction would in general mean that we would no
longer be able to cache constexpr calls for which RVO possibly applies, because
the result of the call may now depend on the ultimate object under construction.
So as a mostly correct stopgap solution that retains cachability of RVO'd
constexpr calls, this patch fixes this issue by rewriting all occurrences of the
RESULT_DECL in the result of a constexpr function call with the current object
under construction, after the call returns. This means the 'this' pointer
during construction of the temporary will still point to the temporary object
instead of the ultimate object, but besides that this approach seems
functionally equivalent to the proper approach.
Successfully bootstrapped and regtested on x86_64-pc-linux-gnu, does this look
OK to commit?
gcc/cp/ChangeLog:
PR c++/94034
* constexpr.c (replace_result_decl_data): New struct.
(replace_result_decl_data_r): New function.
(replace_result_decl): New function.
(cxx_eval_call_expression): Use it.
* tree.c (build_aggr_init_expr): Propagate the location of the
initializer to the AGGR_INIT_EXPR.
gcc/testsuite/ChangeLog:
PR c++/94034
* g++.dg/cpp1y/constexpr-nsdmi6a.C: New test.
* g++.dg/cpp1y/constexpr-nsdmi6b.C: New test.
* g++.dg/cpp1y/constexpr-nsdmi7a.C: New test.
* g++.dg/cpp1y/constexpr-nsdmi7b.C: New test.
---
gcc/cp/constexpr.c | 51 +++++++++++++++++++
gcc/cp/tree.c | 3 ++
.../g++.dg/cpp1y/constexpr-nsdmi6a.C | 26 ++++++++++
.../g++.dg/cpp1y/constexpr-nsdmi6b.C | 27 ++++++++++
.../g++.dg/cpp1y/constexpr-nsdmi7a.C | 49 ++++++++++++++++++
.../g++.dg/cpp1y/constexpr-nsdmi7b.C | 48 +++++++++++++++++
6 files changed, 204 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C
create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 5793430c88d..4cf5812e8a5 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -2029,6 +2029,49 @@ cxx_eval_dynamic_cast_fn (const constexpr_ctx *ctx, tree call,
return cp_build_addr_expr (obj, complain);
}
+/* Data structure used by replace_result_decl and replace_result_decl_r. */
+
+struct replace_result_decl_data
+{
+ /* The RESULT_DECL we want to replace. */
+ tree decl;
+ /* The replacement for DECL. */
+ tree replacement;
+ /* Whether we've performed any replacements. */
+ bool changed;
+};
+
+/* Helper function for replace_result_decl, called through cp_walk_tree. */
+
+static tree
+replace_result_decl_r (tree *tp, int *walk_subtrees, void *data)
+{
+ replace_result_decl_data *d = (replace_result_decl_data *) data;
+
+ if (*tp == d->decl)
+ {
+ *tp = unshare_expr (d->replacement);
+ d->changed = true;
+ *walk_subtrees = 0;
+ }
+ else if (TYPE_P (*tp))
+ *walk_subtrees = 0;
+
+ return NULL_TREE;
+}
+
+/* Replace every occurrence of DECL, a RESULT_DECL, with (an unshared copy of)
+ REPLACEMENT within *TP. Returns true iff a replacement was performed. */
+
+static bool
+replace_result_decl (tree *tp, tree decl, tree replacement)
+{
+ gcc_assert (TREE_CODE (decl) == RESULT_DECL);
+ replace_result_decl_data data = { decl, replacement, false };
+ cp_walk_tree_without_duplicates (tp, replace_result_decl_r, &data);
+ return data.changed;
+}
+
/* Subroutine of cxx_eval_constant_expression.
Evaluate the call expression tree T in the context of OLD_CALL expression
evaluation. */
@@ -2536,6 +2579,14 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
break;
}
}
+
+ /* Rewrite all occurrences of the function's RESULT_DECL with the
+ current object under construction. */
+ if (ctx->object
+ && (same_type_ignoring_top_level_qualifiers_p
+ (TREE_TYPE (res), TREE_TYPE (ctx->object))))
+ if (replace_result_decl (&result, res, ctx->object))
+ cacheable = false;
}
else
/* Couldn't get a function copy to evaluate. */
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index d1192b7e094..1d49d43c229 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -669,6 +669,9 @@ build_aggr_init_expr (tree type, tree init)
else
rval = init;
+ if (location_t loc = EXPR_LOCATION (init))
+ SET_EXPR_LOCATION (rval, loc);
+
return rval;
}
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C
new file mode 100644
index 00000000000..bb844b952e2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6a.C
@@ -0,0 +1,26 @@
+// PR c++/94034
+// { dg-do compile { target c++14 } }
+
+struct A {
+ A *ap = this;
+};
+
+constexpr A foo()
+{
+ return {};
+}
+
+constexpr A bar()
+{
+ return foo();
+}
+
+void
+baz()
+{
+ constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" }
+ constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" }
+}
+
+constexpr A a = foo();
+constexpr A b = bar();
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C
new file mode 100644
index 00000000000..f847fe9809f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi6b.C
@@ -0,0 +1,27 @@
+// PR c++/94034
+// { dg-do compile { target c++14 } }
+
+struct A {
+ A() = default; A(const A&);
+ A *ap = this;
+};
+
+constexpr A foo()
+{
+ return {};
+}
+
+constexpr A bar()
+{
+ return foo();
+}
+
+void
+baz()
+{
+ constexpr A a = foo(); // { dg-error ".A..& a... is not a constant expression" }
+ constexpr A b = bar(); // { dg-error ".A..& b... is not a constant expression" }
+}
+
+constexpr A a = foo();
+constexpr A b = bar();
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C
new file mode 100644
index 00000000000..5a40cd0b845
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7a.C
@@ -0,0 +1,49 @@
+// PR c++/94034
+// { dg-do compile { target c++14 } }
+
+struct A
+{
+ A *p = this;
+ int n = 2;
+ int m = p->n++;
+};
+
+constexpr A
+foo()
+{
+ return {};
+}
+
+constexpr A
+bar()
+{
+ A a = foo();
+ a.p->n = 5;
+ return a;
+}
+
+static_assert(bar().n == 5, "");
+
+constexpr int
+baz()
+{
+ A b = foo();
+ b.p->n = 10;
+ A c = foo();
+ if (c.p->n != 3 || c.p->m != 2)
+ __builtin_abort();
+ bar();
+ return 0;
+}
+
+static_assert(baz() == 0, "");
+
+constexpr int
+quux()
+{
+ const A d = foo();
+ d.p->n++; // { dg-error "const object" }
+ return 0;
+}
+
+static_assert(quux() == 0, ""); // { dg-error "non-constant" }
diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C
new file mode 100644
index 00000000000..86d8ab4e759
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-nsdmi7b.C
@@ -0,0 +1,48 @@
+// PR c++/94034
+// { dg-do compile { target c++14 } }
+
+struct A
+{
+ A() = default; A(const A&);
+ A *p = this;
+ int n = 2;
+ int m = p->n++;
+};
+
+constexpr A
+foo()
+{
+ return {};
+}
+
+constexpr A
+bar()
+{
+ A a = foo();
+ a.p->n = 5;
+ return a; // { dg-error "non-.constexpr." }
+}
+
+constexpr int
+baz()
+{
+ A b = foo();
+ b.p->n = 10;
+ A c = foo();
+ if (c.p->n != 3 || c.p->m != 2)
+ __builtin_abort();
+ foo();
+ return 0;
+}
+
+static_assert(baz() == 0, "");
+
+constexpr int
+quux()
+{
+ const A d = foo();
+ d.p->n++; // { dg-error "const object" }
+ return 0;
+}
+
+static_assert(quux() == 0, ""); // { dg-error "non-constant" }
--
2.26.0.106.g9fadedd637
More information about the Gcc-patches
mailing list