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

[Jason Merrill <jason@redhat.com>]

On 10/6/22 13:51, Marek Polacek wrote:
> 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;

Yes.

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

Indeed, it can't be triggered by the built-in.  But I see 
ref_conv_binds_directly is also called from warn_for_range_copy, which 
ought to be able to trigger it.

Incidentally, ref_conv_binds_directly should also probably be reversed 
to ref_conv_binds_to_temporary since you can "bind directly" to an 
xvalue that refers to a temporary.

Jason