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?)
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?
>
> > + 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" }
> >
>
>
