Message ID | 20221005212744.640285-1-polacek@redhat.com |
---|---|
State | New |
Headers | show |
Series | c++: fixes for derived-to-base reference binding [PR107085] | expand |
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. 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. > (build_over_call): Don't force_elide when it's a derived-to-base > conversion. > > 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 | 17 +++++++++++++++-- > gcc/testsuite/g++.dg/cpp0x/elision4.C | 17 +++++++++++++++++ > .../ext/reference_constructs_from_temporary1.C | 2 +- > .../ext/reference_converts_from_temporary1.C | 2 +- > 4 files changed, 34 insertions(+), 4 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..15e969d6429 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -9186,7 +9186,11 @@ conv_is_prvalue (conversion *c) > { > if (c->kind == ck_rvalue) > return true; > - if (c->kind == ck_base && c->need_temporary_p) > + if (c->kind == ck_base > + /* We may not need a temporary object for the base itself, but if it > + is the base subobject of a temporary object, we're still dealing > + with a prvalue. */ > + && (c->need_temporary_p || conv_is_prvalue (next_conversion (c)))) > return true; > if (c->kind == ck_user && !TYPE_REF_P (c->type)) > return true; > @@ -9417,7 +9421,16 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > && (DECL_COPY_CONSTRUCTOR_P (fn) > || DECL_MOVE_CONSTRUCTOR_P (fn)) > && !unsafe_return_slot_p (first_arg) > - && conv_binds_ref_to_prvalue (convs[0])) > + && conv_binds_ref_to_prvalue (convs[0]) > + /* Converting a class to another class that then binds to this > + copy/move constructor's argument is OK, but not when it's a > + derived-to-base conversion. */ > + && [convs] { > + for (conversion *t = convs[0]; t; t = next_conversion (t)) > + if (t->kind == ck_base) > + return false; > + return true; > + }()) > { > force_elide = true; > goto not_really_used; > diff --git a/gcc/testsuite/g++.dg/cpp0x/elision4.C b/gcc/testsuite/g++.dg/cpp0x/elision4.C > new file mode 100644 > index 00000000000..4833b50d48e > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/elision4.C > @@ -0,0 +1,17 @@ > +// PR c++/107085 > +// { dg-do compile { target c++11 } } > + > +// Must be non-trivial to exhibit the ICE. > +struct X { > + X(); > + X(X&&); > +}; > +struct Z : X {}; > +X x1 = Z(); > +X x2 = X(Z()); > + > +// ...but let's try the trivial path in build_over_call as well. > +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: e99dcbb54e07b798c3353124f38336f96a826d43
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index bd04a1d309a..15e969d6429 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -9186,7 +9186,11 @@ conv_is_prvalue (conversion *c) { if (c->kind == ck_rvalue) return true; - if (c->kind == ck_base && c->need_temporary_p) + if (c->kind == ck_base + /* We may not need a temporary object for the base itself, but if it + is the base subobject of a temporary object, we're still dealing + with a prvalue. */ + && (c->need_temporary_p || conv_is_prvalue (next_conversion (c)))) return true; if (c->kind == ck_user && !TYPE_REF_P (c->type)) return true; @@ -9417,7 +9421,16 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) && (DECL_COPY_CONSTRUCTOR_P (fn) || DECL_MOVE_CONSTRUCTOR_P (fn)) && !unsafe_return_slot_p (first_arg) - && conv_binds_ref_to_prvalue (convs[0])) + && conv_binds_ref_to_prvalue (convs[0]) + /* Converting a class to another class that then binds to this + copy/move constructor's argument is OK, but not when it's a + derived-to-base conversion. */ + && [convs] { + for (conversion *t = convs[0]; t; t = next_conversion (t)) + if (t->kind == ck_base) + return false; + return true; + }()) { force_elide = true; goto not_really_used; diff --git a/gcc/testsuite/g++.dg/cpp0x/elision4.C b/gcc/testsuite/g++.dg/cpp0x/elision4.C new file mode 100644 index 00000000000..4833b50d48e --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/elision4.C @@ -0,0 +1,17 @@ +// PR c++/107085 +// { dg-do compile { target c++11 } } + +// Must be non-trivial to exhibit the ICE. +struct X { + X(); + X(X&&); +}; +struct Z : X {}; +X x1 = Z(); +X x2 = X(Z()); + +// ...but let's try the trivial path in build_over_call as well. +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]>));