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

[Jason Merrill <jason@redhat.com>]

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.

> +}
> +
>   /* Return tristate::TS_TRUE if converting EXPR to a reference type TYPE does
>      not involve creating a temporary.  Return tristate::TS_FALSE if converting
>      EXPR to a reference type TYPE binds the reference to a temporary.  If the
> @@ -9230,7 +9251,7 @@ ref_conv_binds_directly (tree type, tree expr, bool direct_init_p /*= false*/)
>   					  /*c_cast_p=*/false, flags, tf_none);
>     tristate ret (tristate::TS_UNKNOWN);
>     if (conv && !conv->bad_p)
> -    ret = tristate (!conv_binds_ref_to_prvalue (conv));
> +    ret = tristate (!conv_binds_ref_to_temporary (conv));
>   
>     /* Free all the conversions we allocated.  */
>     obstack_free (&conversion_obstack, p);
> diff --git a/gcc/testsuite/g++.dg/cpp0x/elision4.C b/gcc/testsuite/g++.dg/cpp0x/elision4.C
> new file mode 100644
> index 00000000000..3cc2e3afa5d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/elision4.C
> @@ -0,0 +1,15 @@
> +// PR c++/107085
> +// { dg-do compile { target c++11 } }
> +
> +struct X {
> +  X();
> +  X(X&&);
> +};
> +struct Z : X {};
> +X x1 = Z();
> +X x2 = X(Z());
> +
> +struct B { };
> +struct D : B { };
> +B b1 = D();
> +B b2 = B(D());
> diff --git a/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C b/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
> index 76de905a35d..5354b1dc4e6 100644
> --- a/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
> +++ b/gcc/testsuite/g++.dg/ext/reference_constructs_from_temporary1.C
> @@ -201,7 +201,7 @@ SA(!__reference_constructs_from_temporary(const int&, H));
>   SA(!__reference_constructs_from_temporary(int&&, G2));
>   SA(!__reference_constructs_from_temporary(const int&, H2));
>   
> -SA(!__reference_constructs_from_temporary(const Base&, Der));
> +SA(__reference_constructs_from_temporary(const Base&, Der));
>   
>   // This fails because std::is_constructible_v<int&&, id<int[3]>> is false.
>   SA(!__reference_constructs_from_temporary(int&&, id<int[3]>));
> diff --git a/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C b/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
> index 90196c38742..e6c159e9b00 100644
> --- a/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
> +++ b/gcc/testsuite/g++.dg/ext/reference_converts_from_temporary1.C
> @@ -201,7 +201,7 @@ SA( __reference_converts_from_temporary(const int&, H));
>   SA(!__reference_converts_from_temporary(int&&, G2));
>   SA(!__reference_converts_from_temporary(const int&, H2));
>   
> -SA(!__reference_converts_from_temporary(const Base&, Der));
> +SA(__reference_converts_from_temporary(const Base&, Der));
>   
>   // This fails because std::is_constructible_v<int&&, id<int[3]>> is false.
>   SA(!__reference_converts_from_temporary(int&&, id<int[3]>));
> 
> base-commit: 3ec926d36fbf7cb3ff45759471139f3a71d1c4de