On 4/12/20 9:43 AM, Patrick Palka wrote:
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))))
When can they have different types?
+ 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" }
