From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH v3] c++: fixes for derived-to-base reference binding [PR107085]
Date: Thu, 6 Oct 2022 18:03:57 -0400 [thread overview]
Message-ID: <0ff24d4e-5b8a-ea4e-7225-13d08ddac589@redhat.com> (raw)
In-Reply-To: <Yz9L+2VE5evyna+Z@redhat.com>
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:
>>>>>>> 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.
>
> 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.
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?
Do you want to try that, or shall I?
>> 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.
>
> Good point, I at least did that.
>
> -- >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 | 33 +++++++++++++++----
> 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, 49 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..5a3d509f6c8 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -9210,15 +9210,36 @@ 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. */
> + return (c->kind == ck_base && conv_is_prvalue (next_conversion (c)));
> +}
> +
> +/* 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 +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/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 90e06f914fb..bceedab5cd6 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: 50c35c691517291dbb77b1661761bc59950ba101
next prev parent reply other threads:[~2022-10-06 22:04 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-05 21:27 [PATCH] " Marek Polacek
2022-10-06 0:25 ` Jason Merrill
2022-10-06 14:49 ` [PATCH v2] " Marek Polacek
2022-10-06 14:58 ` Jason Merrill
2022-10-06 17:51 ` Marek Polacek
2022-10-06 18:00 ` Jason Merrill
2022-10-06 21:43 ` [PATCH v3] " Marek Polacek
2022-10-06 22:03 ` Jason Merrill [this message]
2022-10-07 16:10 ` [PATCH v4] " Marek Polacek
2022-10-07 17:01 ` Jason Merrill
2022-10-07 21:26 ` [PATCH v5] " Marek Polacek
2022-10-07 21:50 ` Jason Merrill
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0ff24d4e-5b8a-ea4e-7225-13d08ddac589@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).