| Message ID | Yz7rBzPwUuBl4VQb@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c++: fixes for derived-to-base reference binding [PR107085] | expand |
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
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
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
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))); +} + /* 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]>));