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

[Jason Merrill <jason@redhat.com>]

On 10/7/22 12:10, Marek Polacek wrote:
> On Thu, Oct 06, 2022 at 06:03:57PM -0400, Jason Merrill wrote:
>> On 10/6/22 17:43, Marek Polacek wrote:
>>> On Thu, Oct 06, 2022 at 02:00:40PM -0400, Jason Merrill wrote:
>>>> 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:
>>>>>>> --- 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.
>>>
>>> Even there, I think, it's not possible to trigger it with, say (d is an
>>> array of D):
>>>
>>>        for (const B &b : C{}.d)
>>>          // ...
>>>
>>> because warn_for_range_copy gets *__for_begin as the EXPR, which means that
>>> we won't get to the TARGET_EXPR.  It's like
>>>
>>>     auto&& __for_begin = C{}.d;
>>>     const B &b = *__for_begin;
>>>
>>> and the conversion warn_for_range_copy sees is D -> const B -> const B&, the
>>> original .u.expr is *__for_begin.
>>
>> Ah, right, and so the C{} temporary gets lifetime-extended and there's no
>> problem to warn about.
>>
>>> I could add some checking assert to conv_binds_ref_to_temporary to see if
>>> we ever encounter a COMPONENT_REF/ARRAY_REF around a TARGET_EXPR...
>>
>> Or try to handle it properly even if we can't exercise the code yet.
> 
> Okay, done.

Thanks.

>> I'm thinking again about warning for e.g.
>>
>> const B& b = frotz(C{}.d);
>>
>> where frotz passes through its reference argument, which becomes a dangling
>> reference.  Handling the C{}.d case seems relevant to such a warning.
>>
>> I wonder how many false positives there would be for warning about this for
>> any function that both takes and returns a reference?
> 
> I suppose we'd have to ask if any (or all?) of the parameters is/are
> a reference, to handle std::max?

I was thinking to warn if, in a context that would extend temporary 
lifetime, a call argument binds a reference to a temporary and the 
function returns a reference.

>> Do you want to try that, or shall I?
> 
> So this is like the first example in https://gcc.gnu.org/PR106393, which
> is assigned to...me.  Let me take a stab at the simpler example.  If I
> can't make any progress within, say, a week, I'll unassign so as not to
> block it.
> 
> Meanwhile, here's the updated patch:
> 
> 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): Rename to...
> 	(ref_conv_binds_to_temporary): ...this.  Use
> 	conv_binds_ref_to_temporary.
> 	* cp-tree.h (ref_conv_binds_directly): Rename to...
> 	(ref_conv_binds_to_temporary): ...this.
> 	* method.cc (ref_xes_from_temporary): Use ref_conv_binds_to_temporary.
> 	* parser.cc (warn_for_range_copy): Likewise.
> 
> 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                                | 45 ++++++++++++++++---
>   gcc/cp/cp-tree.h                              |  2 +-
>   gcc/cp/method.cc                              |  2 +-
>   gcc/cp/parser.cc                              |  5 ++-
>   gcc/testsuite/g++.dg/cpp0x/elision4.C         | 15 +++++++
>   .../reference_constructs_from_temporary1.C    |  2 +-
>   .../ext/reference_converts_from_temporary1.C  |  2 +-
>   7 files changed, 61 insertions(+), 12 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..833cd014b7d 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -9210,15 +9210,48 @@ conv_binds_ref_to_prvalue (conversion *c)
>     return conv_is_prvalue (next_conversion (c));
>   }
>   
> -/* 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
> -   conversion is invalid or bad, return tristate::TS_UNKNOWN.  DIRECT_INIT_P
> +/* 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.  */
> +  if (c->kind != ck_base)
> +    return false;
> +  c = next_conversion (c);
> +  if (c->kind == ck_identity && c->u.expr)
> +    {
> +      if (TREE_CODE (c->u.expr) == TARGET_EXPR)
> +	return true;
> +      if ((TREE_CODE (c->u.expr) == COMPONENT_REF
> +	   || TREE_CODE (c->u.expr) == ARRAY_REF)

This should be a loop.

> +	  && TREE_CODE (TREE_OPERAND (c->u.expr, 0)) == TARGET_EXPR)
> +	return true;
> +    }
> +  return false;
> +}
> +
> +/* Return tristate::TS_TRUE if converting EXPR to a reference type TYPE binds
> +   the reference to a temporary.  Return tristate::TS_FALSE if converting
> +   EXPR to a reference type TYPE doesn't bind the reference to a temporary.  If
> +   the conversion is invalid or bad, return tristate::TS_UNKNOWN.  DIRECT_INIT_P
>      says whether the conversion should be done in direct- or copy-initialization
>      context.  */
>   
>   tristate
> -ref_conv_binds_directly (tree type, tree expr, bool direct_init_p /*= false*/)
> +ref_conv_binds_to_temporary (tree type, tree expr, bool direct_init_p/*=false*/)
>   {
>     gcc_assert (TYPE_REF_P (type));
>   
> @@ -9230,7 +9263,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/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 8bc1c2dc7fd..469eb2fdb25 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -6534,7 +6534,7 @@ extern bool sufficient_parms_p			(const_tree);
>   extern tree type_decays_to			(tree);
>   extern tree extract_call_expr			(tree);
>   extern tree build_trivial_dtor_call		(tree, bool = false);
> -extern tristate ref_conv_binds_directly		(tree, tree, bool = false);
> +extern tristate ref_conv_binds_to_temporary	(tree, tree, bool = false);
>   extern tree build_user_type_conversion		(tree, tree, int,
>   						 tsubst_flags_t);
>   extern tree build_new_function_call		(tree, vec<tree, va_gc> **,
> diff --git a/gcc/cp/method.cc b/gcc/cp/method.cc
> index 55af5c43c18..622e1b9802e 100644
> --- a/gcc/cp/method.cc
> +++ b/gcc/cp/method.cc
> @@ -2233,7 +2233,7 @@ ref_xes_from_temporary (tree to, tree from, bool direct_init_p)
>     tree val = build_stub_object (from);
>     if (!TYPE_REF_P (from) && TREE_CODE (from) != FUNCTION_TYPE)
>       val = CLASS_TYPE_P (from) ? force_rvalue (val, tf_none) : rvalue (val);
> -  return ref_conv_binds_directly (to, val, direct_init_p).is_false ();
> +  return ref_conv_binds_to_temporary (to, val, direct_init_p).is_true ();
>   }
>   
>   /* Worker for is_{,nothrow_}convertible.  Attempt to perform an implicit
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 555476e42e7..dc3d17c416c 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -13731,7 +13731,8 @@ warn_for_range_copy (tree decl, tree expr)
>   
>     if (TYPE_REF_P (type))
>       {
> -      if (glvalue_p (expr) && ref_conv_binds_directly (type, expr).is_false ())
> +      if (glvalue_p (expr)
> +	  && ref_conv_binds_to_temporary (type, expr).is_true ())
>   	{
>   	  auto_diagnostic_group d;
>   	  if (warning_at (loc, OPT_Wrange_loop_construct,
> @@ -13762,7 +13763,7 @@ warn_for_range_copy (tree decl, tree expr)
>     /* If we can initialize a reference directly, suggest that to avoid the
>        copy.  */
>     tree rtype = cp_build_reference_type (type, /*rval*/false);
> -  if (ref_conv_binds_directly (rtype, expr).is_true ())
> +  if (ref_conv_binds_to_temporary (rtype, expr).is_false ())
>       {
>         auto_diagnostic_group d;
>         if (warning_at (loc, OPT_Wrange_loop_construct,
> 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: f8ba88b6a811ca9bb4b8411d3f65c329fb480ee1