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<type>())
{
// This construction might throw:
variant __tmp(in_place_index<_Np>, __il,
std::forward<_Args>(__args)...);
// But _Never_valueless_alt<type> 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<basic_string<C,T,A>> incorrectly
assumed that is_nothrow_move_constructible<basic_string<C,T,A>> 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<false, T...>::_M_valid()
was not taking advantage of the __never_valueless<T...>() function to
avoid a runtime check. Only the _Variant_storage<true, T...>::_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<false, T...>::_M_valid()):
Check __never_valueless<T...>().
(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.
Tested powerpc64le-linux, committed to trunk.
commit d97dc3efa90199d57c106942e7bf5ba122d963a9
Author: Jonathan Wakely <jwak...@redhat.com>
Date: Thu Apr 18 14:45:40 2019 +0100
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<type>())
{
// This construction might throw:
variant __tmp(in_place_index<_Np>, __il,
std::forward<_Args>(__args)...);
// But _Never_valueless_alt<type> 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<basic_string<C,T,A>> incorrectly
assumed that is_nothrow_move_constructible<basic_string<C,T,A>> 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<false, T...>::_M_valid()
was not taking advantage of the __never_valueless<T...>() function to
avoid a runtime check. Only the _Variant_storage<true, T...>::_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<false, T...>::_M_valid()):
Check __never_valueless<T...>().
(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.
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<typename> struct _Never_valueless_alt; // see <variant>
// 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<typename _Tp, typename _Traits, typename _Alloc>
struct _Never_valueless_alt<std::basic_string<_Tp, _Traits, _Alloc>>
- : std::is_nothrow_move_assignable<std::basic_string<_Tp, _Traits,
_Alloc>>
+ : __and_<
+ is_nothrow_move_constructible<std::basic_string<_Tp, _Traits, _Alloc>>,
+ is_nothrow_move_assignable<std::basic_string<_Tp, _Traits, _Alloc>>
+ >::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<T> 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 <typename... _Types>
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<type>())
+ else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
+ && _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<type>())
+ else if constexpr (__detail::__variant::_Never_valueless_alt<type>()
+ && _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<variant<int>, 0>(0));
static_assert(!has_type_emplace<variant<AllDeleted>, AllDeleted>(0));
static_assert(!has_index_emplace<variant<AllDeleted>, 0>(0));
+ static_assert(has_type_emplace<variant<int, AllDeleted>, int>(0));
+ static_assert(has_index_emplace<variant<int, AllDeleted>, 0>(0));
+ static_assert(has_type_emplace<variant<int, vector<int>, AllDeleted>,
vector<int>>(0));
+ static_assert(has_index_emplace<variant<int, vector<int>, AllDeleted>,
1>(0));
+
+ // The above tests only check the emplace members are available for
+ // overload resolution. The following odr-uses will instantiate them:
+ variant<int, vector<int>, AllDeleted> v;
+ v.emplace<0>(1);
+ v.emplace<int>(1);
+ v.emplace<1>(1, 1);
+ v.emplace<vector<int>>(1, 1);
+ v.emplace<1>({1, 2, 3, 4});
+ v.emplace<vector<int>>({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 <vector>
#include <unordered_set>
#include <memory_resource>
+#include <ext/throw_allocator.h>
#include <testsuite_hooks.h>
using namespace std;
@@ -254,6 +255,41 @@ void emplace()
VERIFY(&v.emplace<0>({1,2,3}) == &std::get<0>(v));
VERIFY(&v.emplace<vector<int>>({1,2,3}) == &std::get<vector<int>>(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<vector<AlwaysThrow>>::value);
+ variant<int, DeletedMoves, vector<AlwaysThrow>> v;
+ v.emplace<2>(1);
+ v.emplace<vector<AlwaysThrow>>(1);
+ v.emplace<0>(0);
+
+ // To test the emplace(initializer_list<U>, Args&&...) members we
+ // can't use AlwaysThrow because elements in an initialier_list
+ // are always copied. Use throw_allocator instead.
+ using Vector = vector<int, __gnu_cxx::throw_allocator_limit<int>>;
+ // static_assert(__detail::__variant::_Never_valueless_alt<Vector>::value);
+ variant<int, DeletedMoves, Vector> vv;
+ Vector::allocator_type::set_limit(1);
+ vv.emplace<2>(1, 1);
+ Vector::allocator_type::set_limit(1);
+ vv.emplace<Vector>(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<Vector>({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()
