[PATCH] c++: Stray RESULT_DECLs in result of constexpr function call [PR94034]

[Patrick Palka]

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} { }
    long d = 0;
  };

  constexpr A baz(const A *q) { return A(p); };
  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'.