On Tue, 14 Apr 2020, Jason Merrill wrote:
> On 4/14/20 9:01 AM, Patrick Palka wrote:
> > On Tue, 14 Apr 2020, Patrick Palka wrote:
> >
> > > 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;
> >
> > We should probably require !*non_constant_p here before doing the
> > replacement because it doesn't make sense to do this transformation for
> > non-constant calls to constexpr functions.
>
> Sounds good.
>
> > And requiring !*non_constant_p means that the result must be a reduced
> > constant expression
>
> No, it doesn't: it means that the result doesn't contain anything that makes
> it not a valid sub- constant expression. Indeed, if there's a RESULT_DECL in
> there that makes it not a reduced constant expression.
Ah okay, that makes sense.
I'll add the !*non_constant_p check to the approved patch and commit
after another round of testing.
>
> Jason
