From 47a468bdbe67a2202f470900de27c5dc98a7e3e0 Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Tue, 23 Apr 2019 10:55:33 +0100 Subject: [PATCH] Fix std::variant regression caused by never-valueless optimization A regression was introduced by the recent changes to provide the strong exception safety guarantee for "never valueless" types that have O(1), non-throwing move assignment. The problematic code is: else if constexpr (__detail::__variant::_Never_valueless_alt()) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, std::forward<_Args>(__args)...); // But _Never_valueless_alt means this won't: *this = std::move(__tmp); } When the variant is not assignable, the assignment is ill-formed, so should not be attempted. When the variant has a copy assignment operator but not a move assignment operator, the assignment performs a copy assignment and that could throw, so should not be attempted. The solution is to only take that branch when the variant has a move assignment operator, which is determined by the _Traits::_S_move_assign constant. When that is false the strong exception safety guarantee is not possible, and so the __never_valueless function should also depend on _S_move_assign. While testing the fixes for this I noticed that the partial specialization _Never_valueless_alt> incorrectly assumed that is_nothrow_move_constructible> is always true, but that's wrong for fully-dynamic COW strings. Fix the partial specialization, and improve the comment describing _Never_valueless_alt to be clear it depends on move construction as well as move assignment. Finally, I also observed that _Variant_storage::_M_valid() was not taking advantage of the __never_valueless() function to avoid a runtime check. Only the _Variant_storage::_M_valid() function was using __never_valueless. That is also fixed. PR libstdc++/87431 * include/bits/basic_string.h (_Never_valueless_alt): Make partial specialization also depend on is_nothrow_move_constructible. * include/std/variant (__detail::__variant::__never_valueless()): Only true if the variant would have a move assignment operator. (__detail::__variant::_Variant_storage::_M_valid()): Check __never_valueless(). (variant::emplace): Only perform non-throwing move assignments for never-valueless alternatives if the variant has a move assignment operator. * testsuite/20_util/variant/compile.cc: Check that never-valueless types can be emplaced into non-assignable variants. * testsuite/20_util/variant/run.cc: Check that never-valueless types don't get copied when emplaced into non-assignable variants. From-SVN: r270502 --- libstdc++-v3/ChangeLog | 15 ++++++++ libstdc++-v3/include/bits/basic_string.h | 7 ++-- libstdc++-v3/include/std/variant | 17 +++++---- .../testsuite/20_util/variant/compile.cc | 14 ++++++++ libstdc++-v3/testsuite/20_util/variant/run.cc | 36 +++++++++++++++++++ 5 files changed, 81 insertions(+), 8 deletions(-) diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 9dad28f502a..0949c94ae19 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,5 +1,20 @@ 2019-04-23 Jonathan Wakely + PR libstdc++/87431 + * include/bits/basic_string.h (_Never_valueless_alt): Make partial + specialization also depend on is_nothrow_move_constructible. + * include/std/variant (__detail::__variant::__never_valueless()): + Only true if the variant would have a move assignment operator. + (__detail::__variant::_Variant_storage::_M_valid()): + Check __never_valueless(). + (variant::emplace): Only perform non-throwing move assignments + for never-valueless alternatives if the variant has a move assignment + operator. + * testsuite/20_util/variant/compile.cc: Check that never-valueless + types can be emplaced into non-assignable variants. + * testsuite/20_util/variant/run.cc: Check that never-valueless types + don't get copied when emplaced into non-assignable variants. + * include/std/variant (__detail::__variant::__ref_cast): Remove unused function. (__detail::__variant::_Uninitialized::_M_get) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 20a56277a57..922536965e3 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -6849,10 +6849,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template struct _Never_valueless_alt; // see // Provide the strong exception-safety guarantee when emplacing a - // basic_string into a variant, but only if move assignment cannot throw. + // basic_string into a variant, but only if moving the string cannot throw. template struct _Never_valueless_alt> - : std::is_nothrow_move_assignable> + : __and_< + is_nothrow_move_constructible>, + is_nothrow_move_assignable> + >::type { }; } // namespace __detail::__variant #endif // C++17 diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index 08378eee816..b71bb027f2b 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -340,9 +340,9 @@ namespace __variant { }; // Specialize _Never_valueless_alt for other types which have a - // non-throwing and cheap move assignment operator, so that emplacing - // the type will provide the strong exception-safety guarantee, - // by creating and moving a temporary. + // non-throwing and cheap move construction and move assignment operator, + // so that emplacing the type will provide the strong exception-safety + // guarantee, by creating and moving a temporary. // Whether _Never_valueless_alt is true or not affects the ABI of a // variant using that alternative, so we can't change the value later! @@ -352,7 +352,8 @@ namespace __variant template constexpr bool __never_valueless() { - return (_Never_valueless_alt<_Types>::value && ...); + return _Traits<_Types...>::_S_move_assign + && (_Never_valueless_alt<_Types>::value && ...); } // Defines index and the dtor, possibly trivial. @@ -407,6 +408,8 @@ namespace __variant constexpr bool _M_valid() const noexcept { + if constexpr (__never_valueless<_Types...>()) + return true; return this->_M_index != __index_type(variant_npos); } @@ -1428,7 +1431,8 @@ namespace __variant this->_M_reset(); __variant_construct_by_index<_Np>(*this, __tmp); } - else if constexpr (__detail::__variant::_Never_valueless_alt()) + else if constexpr (__detail::__variant::_Never_valueless_alt() + && _Traits::_S_move_assign) { // This construction might throw: variant __tmp(in_place_index<_Np>, @@ -1474,7 +1478,8 @@ namespace __variant __variant_construct_by_index<_Np>(*this, __il, std::forward<_Args>(__args)...); } - else if constexpr (__detail::__variant::_Never_valueless_alt()) + else if constexpr (__detail::__variant::_Never_valueless_alt() + && _Traits::_S_move_assign) { // This construction might throw: variant __tmp(in_place_index<_Np>, __il, diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc index 5cc2a9460a9..afd593d2fef 100644 --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc @@ -479,6 +479,20 @@ void test_emplace() static_assert(has_index_emplace, 0>(0)); static_assert(!has_type_emplace, AllDeleted>(0)); static_assert(!has_index_emplace, 0>(0)); + static_assert(has_type_emplace, int>(0)); + static_assert(has_index_emplace, 0>(0)); + static_assert(has_type_emplace, AllDeleted>, vector>(0)); + static_assert(has_index_emplace, AllDeleted>, 1>(0)); + + // The above tests only check the emplace members are available for + // overload resolution. The following odr-uses will instantiate them: + variant, AllDeleted> v; + v.emplace<0>(1); + v.emplace(1); + v.emplace<1>(1, 1); + v.emplace>(1, 1); + v.emplace<1>({1, 2, 3, 4}); + v.emplace>({1, 2, 3, 4}); } void test_triviality() diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc index c0f48432ca1..ec1e86805cd 100644 --- a/libstdc++-v3/testsuite/20_util/variant/run.cc +++ b/libstdc++-v3/testsuite/20_util/variant/run.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include using namespace std; @@ -254,6 +255,41 @@ void emplace() VERIFY(&v.emplace<0>({1,2,3}) == &std::get<0>(v)); VERIFY(&v.emplace>({1,2,3}) == &std::get>(v)); } + + { + // Ensure no copies of the vector are made, only moves. + // See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87431#c21 + + // static_assert(__detail::__variant::_Never_valueless_alt>::value); + variant> v; + v.emplace<2>(1); + v.emplace>(1); + v.emplace<0>(0); + + // To test the emplace(initializer_list, Args&&...) members we + // can't use AlwaysThrow because elements in an initialier_list + // are always copied. Use throw_allocator instead. + using Vector = vector>; + // static_assert(__detail::__variant::_Never_valueless_alt::value); + variant vv; + Vector::allocator_type::set_limit(1); + vv.emplace<2>(1, 1); + Vector::allocator_type::set_limit(1); + vv.emplace(1, 1); + Vector::allocator_type::set_limit(1); + vv.emplace<0>(0); + Vector::allocator_type::set_limit(1); + vv.emplace<2>({1, 2, 3}); + Vector::allocator_type::set_limit(1); + vv.emplace({1, 2, 3, 4}); + try { + Vector::allocator_type::set_limit(0); + vv.emplace<2>(1, 1); + VERIFY(false); + } catch (__gnu_cxx::forced_error) { + } + VERIFY(vv.valueless_by_exception()); + } } void test_get()