Re: [PATCH v2] c++: fixes for derived-to-base reference binding [PR107085]

[Marek Polacek <polacek@redhat.com>]

On Thu, Oct 06, 2022 at 10:58:44AM -0400, Jason Merrill wrote:
> On 10/6/22 10:49, Marek Polacek wrote:
> > On Wed, Oct 05, 2022 at 08:25:29PM -0400, Jason Merrill wrote:
> > > On 10/5/22 17:27, Marek Polacek wrote:
> > > > This PR reports that
> > > > 
> > > >     struct Base {};
> > > >     struct Derived : Base {};
> > > >     static_assert(__reference_constructs_from_temporary(Base const&, Derived));
> > > > 
> > > > doesn't pass, which it should: it's just like
> > > > 
> > > >     const Base& b(Derived{});
> > > > 
> > > > where we bind 'b' to the Base subobject of a temporary object of type
> > > > Derived.  The ck_base conversion didn't have ->need_temporary_p set because
> > > > we didn't need to create a temporary object just for the base, but the whole
> > > > object is a temporary so we're still binding to a temporary.  Fixed by
> > > > the conv_is_prvalue hunk.
> > > > 
> > > > That broke a bunch of tests.  I've distilled the issue into a simple test
> > > > in elision4.C.  Essentially, we have
> > > > 
> > > >     struct B { /* ... */ };
> > > >     struct D : B { };
> > > >     B b = D();
> > > > 
> > > > and we set force_elide in build_over_call, but we're unable to actually
> > > > elide the B::B(B&&) call, and crash on gcc_assert (!force_elide);.
> > > > 
> > > > <https://en.cppreference.com/w/cpp/language/copy_elision> says that copy
> > > > elision "can only apply when the object being initialized is known not to be
> > > > a potentially-overlapping subobject".  So I suppose we shouldn't force_elide
> > > > the B::B(B&&) call.  I don't belive the CWG 2327 code was added to handle
> > > > derived-to-base conversions, at that time conv_binds_ref_to_prvalue wasn't
> > > > checking ck_base at all.
> > > > 
> > > > Does that make sense?  If so...
> > > > 
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > 
> > > > 	PR c++/107085
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	* call.cc (conv_is_prvalue): Return true if the base subobject is part
> > > > 	of a temporary object.
> > > 
> > > No, the base subobject of a prvalue is an xvalue.
> > 
> > Ah, so this is just like T().m where T() is a prvalue but the whole thing
> > is an xvalue.  Duly noted.
> 
> Exactly.
> 
> > > I think the problem is that an expression being a prvalue is a subset of
> > > binding a reference to a temporary, and we shouldn't try to express both of
> > > those using the same function: you need a separate
> > > conv_binds_ref_to_temporary.
> > 
> > Ack, so how about this?  Thanks,
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > -- >8 --
> > This PR reports that
> > 
> >    struct Base {};
> >    struct Derived : Base {};
> >    static_assert(__reference_constructs_from_temporary(Base const&, Derived));
> > 
> > doesn't pass, which it should: it's just like
> > 
> >    const Base& b(Derived{});
> > 
> > where we bind 'b' to the Base subobject of a temporary object of type
> > Derived.  The ck_base conversion didn't have ->need_temporary_p set because
> > we didn't need to create a temporary object just for the base, but the whole
> > object is a temporary so we're still binding to a temporary.  Since the
> > Base subobject is an xvalue, a new function is introduced.
> > 
> > 	PR c++/107085
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.cc (conv_binds_ref_to_temporary): New.
> > 	(ref_conv_binds_directly): Use it.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/ext/reference_constructs_from_temporary1.C: Adjust expected
> > 	result.
> > 	* g++.dg/ext/reference_converts_from_temporary1.C: Likewise.
> > 	* g++.dg/cpp0x/elision4.C: New test.
> > ---
> >   gcc/cp/call.cc                                | 23 ++++++++++++++++++-
> >   gcc/testsuite/g++.dg/cpp0x/elision4.C         | 15 ++++++++++++
> >   .../reference_constructs_from_temporary1.C    |  2 +-
> >   .../ext/reference_converts_from_temporary1.C  |  2 +-
> >   4 files changed, 39 insertions(+), 3 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/elision4.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index bd04a1d309a..715a83f5a69 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -9210,6 +9210,27 @@ conv_binds_ref_to_prvalue (conversion *c)
> >     return conv_is_prvalue (next_conversion (c));
> >   }
> > +/* True iff C is a conversion that binds a reference to a temporary.
> > +   This is a superset of conv_binds_ref_to_prvalue: here we're also
> > +   interested in xvalues.  */
> > +
> > +static bool
> > +conv_binds_ref_to_temporary (conversion *c)
> > +{
> > +  if (conv_binds_ref_to_prvalue (c))
> > +    return true;
> > +  if (c->kind != ck_ref_bind)
> > +    return false;
> > +  c = next_conversion (c);
> > +  /* This is the case for
> > +       struct Base {};
> > +       struct Derived : Base {};
> > +       const Base& b(Derived{});
> > +     where we bind 'b' to the Base subobject of a temporary object of type
> > +     Derived.  The subobject is an xvalue; the whole object is a prvalue.  */
> > +  return (c->kind == ck_base && conv_is_prvalue (next_conversion (c)));
> 
> I think you also want to check for the case of c->u.expr being a
> COMPONENT_REF/ARRAY_REF around a TARGET_EXPR, as you mentioned.

I see.  So this would be achieved using e.g.

struct B { };
struct D : B { };
struct C {
  D d;
};

const B& b = C{}.d;

Except I'm not sure how to trigger this via the built-in, which takes two types.
Am I missing something obvious?

Marek