On Mon, 13 Apr 2020, Jason Merrill wrote:
On 4/13/20 2:49 PM, Patrick Palka wrote:
On Mon, 13 Apr 2020, Jason Merrill wrote:
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?
During prior testing I tried replacing the same_type_p check with an assert,
i.e.:
if (ctx->object
&& AGGREGATE_TYPE_P (TREE_TYPE (res)))
{
gcc_assert (same_type_ignoring_top_level_qualifiers_p
(TREE_TYPE (res), TREE_TYPE (ctx->object));
...
}
and IIRC I was able to trigger the assert only when the type of
ctx->object and of the RESULT_DECL are distinct empty class types.
Here's a testcase:
struct empty1 { };
constexpr empty1 foo1() { return {}; }
struct empty2 { };
constexpr empty2 foo2(empty1) { return {}; }
constexpr empty2 a = foo2(foo1());
The initializer of 'a' has the form
TARGET_EXPR <D.2389, foo2 (foo1 ();, <<< Unknown tree: empty_class_expr
;)>;
and so during evaluation of 'foo1()', ctx->object still points to 'a' and we
trip over the same_type_p assert. (Is it possible that the call to foo1
is missing a TARGET_EXPR?)
Hmm, that would be because of
if (is_empty_class (TREE_TYPE (arg))
&& simple_empty_class_p (TREE_TYPE (arg), arg, INIT_EXPR))
{ > while (TREE_CODE (arg) == TARGET_EXPR)
/* We're disconnecting the initializer from its target,
don't create a temporary. */
arg = TARGET_EXPR_INITIAL (arg);
in build_call_a.
Ah okay.
Either way, it would be safe to skip empty class types. Should we change
the
condition to
if (ctx->object
&& AGGREGATE_TYPE_P (TREE_TYPE (res))
&& !is_empty_class (TREE_TYPE (res)))
{
...
}
and turn the same_type_p check into an assert?
Sounds good. Let's say a checking_assert.
Great, I'm bootstrapping/regtesting the following overnight, does it
look OK to commit if successful?
-- >8 --
Subject: [PATCH] c++: Stray RESULT_DECLs in result of constexpr call [PR94034]
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): Set the location of the AGGR_INIT_EXPR
to that of its initializer.
gcc/testsuite/ChangeLog:
PR c++/94034
* g++.dg/cpp0x/constexpr-empty15.C: New test.
* 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 | 53 +++++++++++++++++++
gcc/cp/tree.c | 3 ++
.../g++.dg/cpp0x/constexpr-empty15.C | 9 ++++
.../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 +++++++++++++++++
7 files changed, 215 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-empty15.C
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 d8636ddb92f..00e313178f9 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -2029,6 +2029,51 @@ 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_checking_assert (TREE_CODE (decl) == RESULT_DECL
+ && (same_type_ignoring_top_level_qualifiers_p
+ (TREE_TYPE (decl), TREE_TYPE (replacement))));
+ 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 +2581,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
+ && AGGREGATE_TYPE_P (TREE_TYPE (res))
+ && !is_empty_class (TREE_TYPE (res)))
+ if (replace_result_decl (&result, res, ctx->object))
+ cacheable = false;
