From 10f26de9155b71a2bd5055060004420939cf7a2d Mon Sep 17 00:00:00 2001 From: Jonathan Wakely Date: Fri, 5 Apr 2019 17:56:09 +0100 Subject: [PATCH] PR libstdc++/87431 re-adjust never-valueless optimizations Avoid creating arbitrarily large objects on the stack when emplacing trivially copyable objects into a variant. Currently we provide the strong exception-safety guarantee for all trivially copyable types, by constructing a second variant and then doing a non-throwing move assignment from the temporary. This patch restricts that behaviour to trivially copyable types that are no larger than 256 bytes. For larger types the object will be emplaced directly into the variant, and if its initialization throws then the variant becomes valueless. Also implement Antony Polukhin's suggestion to whitelist specific types that are not trivially copyable but can be efficiently move-assigned. Emplacing those types will never cause a variant to become valueless. The whitelisted types are: std::shared_ptr, std::weak_ptr, std::unique_ptr, std::function, and std::any. Additionally, std::basic_string, std::vector, and __gnu_debug::vector are whitelisted if their allocator traits give them a non-throwing move assignment operator. Specifically, this means std::string is whitelisted, but std::pmr::string is not. As part of this patch, additional if-constexpr branches are added for the cases where the initialization is known to be non-throwing (so the overhead of the try-catch block can be avoided) and where a scalar is being produced by a potentially-throwing conversion operator (so that the overhead of constructing and move-assigning a variant is avoided). These changes should have no semantic effect, just better codegen. PR libstdc++/87431 (again) * include/bits/basic_string.h (__variant::_Never_valueless_alt): Define partial specialization for basic_string. * include/bits/shared_ptr.h (_Never_valueless_alt): Likewise for shared_ptr and weak_ptr. * include/bits/std_function.h (_Never_valueless_alt): Likewise for function. * include/bits/stl_vector.h (_Never_valueless_alt): Likewise for vector. * include/bits/unique_ptr.h (_Never_valueless_alt): Likewise for unique_ptr. * include/debug/vector (_Never_valueless_alt): Likewise for debug vector. * include/std/any (_Never_valueless_alt): Define explicit specialization for any. * include/std/variant (_Never_valueless_alt): Define primary template. (__never_valueless): Use _Never_valueless_alt instead of is_trivially_copyable. (variant::emplace(Args&&...)): Add special case for non-throwing initializations to avoid try-catch overhead. Add special case for scalars produced by potentially-throwing conversions. Use _Never_valueless_alt instead of is_trivially_copyable for the remaining strong exception-safety cases. (variant::emplace(initializer_list, Args&&...)): Likewise. * testsuite/20_util/variant/87431.cc: Run both test functions. * testsuite/20_util/variant/exception_safety.cc: New test. * testsuite/20_util/variant/run.cc: Use pmr::string instead of string, so the variant becomes valueless. From-SVN: r270170 --- libstdc++-v3/ChangeLog | 31 +++ libstdc++-v3/include/bits/basic_string.h | 17 +- libstdc++-v3/include/bits/shared_ptr.h | 21 +++ libstdc++-v3/include/bits/std_function.h | 15 +- libstdc++-v3/include/bits/stl_vector.h | 15 ++ libstdc++-v3/include/bits/unique_ptr.h | 14 ++ libstdc++-v3/include/debug/vector | 14 ++ libstdc++-v3/include/std/any | 15 +- libstdc++-v3/include/std/variant | 145 +++++++++----- .../testsuite/20_util/variant/87431.cc | 1 + .../20_util/variant/exception_safety.cc | 178 ++++++++++++++++++ libstdc++-v3/testsuite/20_util/variant/run.cc | 7 +- 12 files changed, 415 insertions(+), 58 deletions(-) create mode 100644 libstdc++-v3/testsuite/20_util/variant/exception_safety.cc diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index e2038875220..a34428f762b 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -1,3 +1,34 @@ +2019-04-05 Jonathan Wakely + + PR libstdc++/87431 (again) + * include/bits/basic_string.h (__variant::_Never_valueless_alt): + Define partial specialization for basic_string. + * include/bits/shared_ptr.h (_Never_valueless_alt): Likewise for + shared_ptr and weak_ptr. + * include/bits/std_function.h (_Never_valueless_alt): Likewise for + function. + * include/bits/stl_vector.h (_Never_valueless_alt): Likewise for + vector. + * include/bits/unique_ptr.h (_Never_valueless_alt): Likewise for + unique_ptr. + * include/debug/vector (_Never_valueless_alt): Likewise for debug + vector. + * include/std/any (_Never_valueless_alt): Define explicit + specialization for any. + * include/std/variant (_Never_valueless_alt): Define primary template. + (__never_valueless): Use _Never_valueless_alt instead of + is_trivially_copyable. + (variant::emplace(Args&&...)): Add special case for non-throwing + initializations to avoid try-catch overhead. Add special case for + scalars produced by potentially-throwing conversions. Use + _Never_valueless_alt instead of is_trivially_copyable for the + remaining strong exception-safety cases. + (variant::emplace(initializer_list, Args&&...)): Likewise. + * testsuite/20_util/variant/87431.cc: Run both test functions. + * testsuite/20_util/variant/exception_safety.cc: New test. + * testsuite/20_util/variant/run.cc: Use pmr::string instead of string, + so the variant becomes valueless. + 2019-04-03 Jonathan Wakely PR libstdc++/85184 diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 4d0894be99b..20a56277a57 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -6800,7 +6800,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION struct __is_fast_hash> : std::false_type { }; -#if __cplusplus > 201103L +#if __cplusplus >= 201402L #define __cpp_lib_string_udls 201304 @@ -6843,7 +6843,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } // inline namespace string_literals } // inline namespace literals -#endif // __cplusplus > 201103L +#if __cplusplus >= 201703L + namespace __detail::__variant + { + 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. + template + struct _Never_valueless_alt> + : std::is_nothrow_move_assignable> + { }; + } // namespace __detail::__variant +#endif // C++17 +#endif // C++14 _GLIBCXX_END_NAMESPACE_VERSION } // namespace std diff --git a/libstdc++-v3/include/bits/shared_ptr.h b/libstdc++-v3/include/bits/shared_ptr.h index ee815f0d0a1..2d53478f1f4 100644 --- a/libstdc++-v3/include/bits/shared_ptr.h +++ b/libstdc++-v3/include/bits/shared_ptr.h @@ -732,6 +732,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // @} group pointer_abstractions +#if __cplusplus >= 201703L + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing a + // shared_ptr into a variant. + template + struct _Never_valueless_alt> + : std::true_type + { }; + + // Provide the strong exception-safety guarantee when emplacing a + // weak_ptr into a variant. + template + struct _Never_valueless_alt> + : std::true_type + { }; + } // namespace __detail::__variant +#endif // C++17 + _GLIBCXX_END_NAMESPACE_VERSION } // namespace diff --git a/libstdc++-v3/include/bits/std_function.h b/libstdc++-v3/include/bits/std_function.h index caa0fd816af..b70ed564d11 100644 --- a/libstdc++-v3/include/bits/std_function.h +++ b/libstdc++-v3/include/bits/std_function.h @@ -787,9 +787,22 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION swap(function<_Res(_Args...)>& __x, function<_Res(_Args...)>& __y) noexcept { __x.swap(__y); } +#if __cplusplus >= 201703L + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing a + // function into a variant. + template + struct _Never_valueless_alt> + : std::true_type + { }; + } // namespace __detail::__variant +#endif // C++17 + _GLIBCXX_END_NAMESPACE_VERSION } // namespace std #endif // C++11 - #endif // _GLIBCXX_STD_FUNCTION_H diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h index 10bf4fac62e..dd9382d254d 100644 --- a/libstdc++-v3/include/bits/stl_vector.h +++ b/libstdc++-v3/include/bits/stl_vector.h @@ -1938,6 +1938,21 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER { __x.swap(__y); } _GLIBCXX_END_NAMESPACE_CONTAINER + +#if __cplusplus >= 201703L + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing a + // vector into a variant, but only if move assignment cannot throw. + template + struct _Never_valueless_alt<_GLIBCXX_STD_C::vector<_Tp, _Alloc>> + : std::is_nothrow_move_assignable<_GLIBCXX_STD_C::vector<_Tp, _Alloc>> + { }; + } // namespace __detail::__variant +#endif // C++17 + _GLIBCXX_END_NAMESPACE_VERSION } // namespace std diff --git a/libstdc++-v3/include/bits/unique_ptr.h b/libstdc++-v3/include/bits/unique_ptr.h index 61a3ef05460..963e4eb7d44 100644 --- a/libstdc++-v3/include/bits/unique_ptr.h +++ b/libstdc++-v3/include/bits/unique_ptr.h @@ -866,6 +866,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION // @} group pointer_abstractions +#if __cplusplus >= 201703L + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing a + // unique_ptr into a variant. + template + struct _Never_valueless_alt> + : std::true_type + { }; + } // namespace __detail::__variant +#endif // C++17 + _GLIBCXX_END_NAMESPACE_VERSION } // namespace diff --git a/libstdc++-v3/include/debug/vector b/libstdc++-v3/include/debug/vector index 6d9536ee73a..56b5e140fd3 100644 --- a/libstdc++-v3/include/debug/vector +++ b/libstdc++-v3/include/debug/vector @@ -801,6 +801,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Sequence, std::random_access_iterator_tag>& __it) { return std::__niter_base(__it.base()); } +#if __cplusplus >= 201703L + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing a + // vector into a variant, but only if move assignment cannot throw. + template + struct _Never_valueless_alt<__debug::vector<_Tp, _Alloc>> + : std::is_nothrow_move_assignable<__debug::vector<_Tp, _Alloc>> + { }; + } // namespace __detail::__variant +#endif // C++17 + _GLIBCXX_END_NAMESPACE_VERSION } // namespace std diff --git a/libstdc++-v3/include/std/any b/libstdc++-v3/include/std/any index 385a99ce744..b0553dccf22 100644 --- a/libstdc++-v3/include/std/any +++ b/libstdc++-v3/include/std/any @@ -614,9 +614,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION /// @} + namespace __detail::__variant + { + template struct _Never_valueless_alt; // see + + // Provide the strong exception-safety guarantee when emplacing an + // any into a variant. + template<> + struct _Never_valueless_alt + : std::true_type + { }; + } // namespace __detail::__variant + _GLIBCXX_END_NAMESPACE_VERSION } // namespace std -#endif // C++14 - +#endif // C++17 #endif // _GLIBCXX_ANY diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index a21ef3005cf..e52aa403009 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -327,11 +327,31 @@ namespace __variant _Variadic_union<_Rest...> _M_rest; }; + // _Never_valueless_alt is true for variant alternatives that can + // always be placed in a variant without it becoming valueless. + + // For suitably-small, trivially copyable types we can create temporaries + // on the stack and then memcpy them into place. + template + struct _Never_valueless_alt + : __and_, is_trivially_copyable<_Tp>> + { }; + + // 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. + // 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! + + // True if every alternative in _Types... can be emplaced in a variant + // without it becoming valueless. If this is true, variant<_Types...> + // can never be valueless, which enables some minor optimizations. template - constexpr bool __never_valueless() - { - return (is_trivially_copyable_v<_Types> && ...); - } + constexpr bool __never_valueless() + { + return (_Never_valueless_alt<_Types>::value && ...); + } // Defines index and the dtor, possibly trivial. template @@ -776,19 +796,19 @@ namespace __variant { return __v._M_storage(); } template - struct _Extra_visit_slot_needed - { - template struct _Variant_never_valueless; + struct _Extra_visit_slot_needed + { + template struct _Variant_never_valueless; - template - struct _Variant_never_valueless> - : bool_constant<__never_valueless<_Types...>()> {}; + template + struct _Variant_never_valueless> + : bool_constant<__never_valueless<_Types...>()> {}; - static constexpr bool value = - (is_same_v<_Maybe_variant_cookie, __variant_cookie> - || is_same_v<_Maybe_variant_cookie, __variant_idx_cookie>) - && !_Variant_never_valueless<__remove_cvref_t<_Variant>>::value; - }; + static constexpr bool value = + (is_same_v<_Maybe_variant_cookie, __variant_cookie> + || is_same_v<_Maybe_variant_cookie, __variant_idx_cookie>) + && !_Variant_never_valueless<__remove_cvref_t<_Variant>>::value; + }; // Used for storing multi-dimensional vtable. template @@ -1329,31 +1349,45 @@ namespace __variant static_assert(_Np < sizeof...(_Types), "The index should be in [0, number of alternatives)"); using type = variant_alternative_t<_Np, variant>; - // If constructing the value can throw but move assigning it can't, - // construct in a temporary and then move assign from it. This gives - // the strong exception safety guarantee, ensuring we never become - // valueless. - if constexpr (is_trivially_copyable_v - && !is_nothrow_constructible_v) + // Provide the strong exception-safety guarantee when possible, + // to avoid becoming valueless. + if constexpr (is_nothrow_constructible_v) + { + this->_M_reset(); + __variant_construct_by_index<_Np>(*this, + std::forward<_Args>(__args)...); + } + else if constexpr (is_scalar_v) + { + // This might invoke a potentially-throwing conversion operator: + const type __tmp(std::forward<_Args>(__args)...); + // But these steps won't throw: + this->_M_reset(); + __variant_construct_by_index<_Np>(*this, __tmp); + } + else if constexpr (__detail::__variant::_Never_valueless_alt()) { - // If move assignment cannot throw then we can provide the - // strong exception safety guarantee, and never become valueless. + // This construction might throw: variant __tmp(in_place_index<_Np>, std::forward<_Args>(__args)...); + // But _Never_valueless_alt means this won't: *this = std::move(__tmp); - return std::get<_Np>(*this); } - - this->_M_reset(); - __try - { - __variant_construct_by_index<_Np>(*this, - std::forward<_Args>(__args)...); - } - __catch (...) + else { - this->_M_index = variant_npos; - __throw_exception_again; + // This case only provides the basic exception-safety guarantee, + // i.e. the variant can become valueless. + this->_M_reset(); + __try + { + __variant_construct_by_index<_Np>(*this, + std::forward<_Args>(__args)...); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } } return std::get<_Np>(*this); } @@ -1367,28 +1401,39 @@ namespace __variant static_assert(_Np < sizeof...(_Types), "The index should be in [0, number of alternatives)"); using type = variant_alternative_t<_Np, variant>; - if constexpr (is_trivially_copyable_v - && !is_nothrow_constructible_v, - _Args...>) + // Provide the strong exception-safety guarantee when possible, + // to avoid becoming valueless. + if constexpr (is_nothrow_constructible_v&, + _Args...>) { - // If move assignment cannot throw then we can provide the - // strong exception safety guarantee, and never become valueless. + this->_M_reset(); + __variant_construct_by_index<_Np>(*this, __il, + std::forward<_Args>(__args)...); + } + 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); - return std::get<_Np>(*this); } - - this->_M_reset(); - __try - { - __variant_construct_by_index<_Np>(*this, __il, - std::forward<_Args>(__args)...); - } - __catch (...) + else { - this->_M_index = variant_npos; - __throw_exception_again; + // This case only provides the basic exception-safety guarantee, + // i.e. the variant can become valueless. + this->_M_reset(); + __try + { + __variant_construct_by_index<_Np>(*this, __il, + std::forward<_Args>(__args)...); + } + __catch (...) + { + this->_M_index = variant_npos; + __throw_exception_again; + } } return std::get<_Np>(*this); } diff --git a/libstdc++-v3/testsuite/20_util/variant/87431.cc b/libstdc++-v3/testsuite/20_util/variant/87431.cc index 8c5c4a78154..bf92564cf92 100644 --- a/libstdc++-v3/testsuite/20_util/variant/87431.cc +++ b/libstdc++-v3/testsuite/20_util/variant/87431.cc @@ -68,4 +68,5 @@ int main() { test01(); + test02(); } diff --git a/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc b/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc new file mode 100644 index 00000000000..7e1b0f3bed1 --- /dev/null +++ b/libstdc++-v3/testsuite/20_util/variant/exception_safety.cc @@ -0,0 +1,178 @@ +// Copyright (C) 2019 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// . + +// { dg-options "-std=gnu++17" } +// { dg-do run { target c++17 } } + +#include +#include +#include +#include +#include +#include +#include +#include + +void +test01() +{ +#if _GLIBCXX_USE_CXX11_ABI + std::variant> v(1); + VERIFY( v.index() == 0 ); + + try + { + std::pmr::string s = "how long is a piece of SSO string?"; + v.emplace<1>(s, std::pmr::null_memory_resource()); + VERIFY( false ); + } + catch(const std::bad_alloc&) + { + VERIFY( v.valueless_by_exception() ); + } + + v.emplace<0>(2); + VERIFY( v.index() == 0 ); + + try + { + v.emplace<2>({1, 2, 3}, std::pmr::null_memory_resource()); + VERIFY( false ); + } + catch(const std::bad_alloc&) + { + VERIFY( v.valueless_by_exception() ); + } +#endif +} + +void +test02() +{ + struct X + { + X(int i) : i(1) { if (i > 2) throw 3; } + X(std::initializer_list l) : i(2) { if (l.size() > 2) throw 3; } + int i; + }; + static_assert( std::is_trivially_copyable_v ); + + std::variant v(111); + VERIFY( v.index() == 1 ); + + try + { + v.emplace(3); + VERIFY( false ); + } + catch(int) + { + VERIFY( !v.valueless_by_exception() ); + VERIFY( v.index() == 1 ); + VERIFY( std::get(v) == 111 ); + } + + v.emplace(1); + VERIFY( v.index() == 2 ); + VERIFY( std::get(v).i == 1 ); + + try + { + v.emplace(3); + VERIFY( false ); + } + catch(int) + { + VERIFY( !v.valueless_by_exception() ); + VERIFY( v.index() == 2 ); + VERIFY( std::get(v).i == 1 ); + } + + try + { + v.emplace({1, 2, 3}); + VERIFY( false ); + } + catch(int) + { + VERIFY( !v.valueless_by_exception() ); + VERIFY( v.index() == 2 ); + VERIFY( std::get(v).i == 1 ); + } +} + +template + bool bad_emplace(V& v) + { + struct X { + operator T() const { throw 1; } + }; + + const auto index = v.index(); + + try + { + if (std::is_same_v) + { + // Need to test std::any differently, because emplace(X{}) + // would create a std::any with a contained X, instead of using + // X::operator any() to convert to std::any. + struct ThrowOnCopy { + ThrowOnCopy() { } + ThrowOnCopy(const ThrowOnCopy&) { throw 1; } + } t; + v.template emplace(t); + } + else + v.template emplace(X{}); + } + catch (int) + { + return v.index() == index; + } + return false; + } + +void +test03() +{ + struct TriviallyCopyable { int i = 0; }; + + std::variant, + std::string, std::vector, std::function, std::any, + std::shared_ptr, std::weak_ptr, std::unique_ptr> v(1); + VERIFY( v.index() == 1 ); + + VERIFY( bad_emplace(v) ); + VERIFY( bad_emplace(v) ); + VERIFY( bad_emplace>(v) ); + VERIFY( bad_emplace(v) ); + VERIFY( bad_emplace>(v) ); + VERIFY( bad_emplace>(v) ); + VERIFY( bad_emplace(v) ); + VERIFY( bad_emplace>(v) ); + VERIFY( bad_emplace>(v) ); + VERIFY( bad_emplace>(v) ); +} + +int +main() +{ + test01(); + test02(); + test03(); +} diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc index 7ee9b08c2ce..3ca375d374e 100644 --- a/libstdc++-v3/testsuite/20_util/variant/run.cc +++ b/libstdc++-v3/testsuite/20_util/variant/run.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include using namespace std; @@ -376,7 +377,7 @@ void test_visit() void test_hash() { - unordered_set> s; + unordered_set> s; VERIFY(s.emplace(3).second); VERIFY(s.emplace("asdf").second); VERIFY(s.emplace().second); @@ -388,12 +389,12 @@ void test_hash() { struct A { - operator string() + operator pmr::string() { throw nullptr; } }; - variant v; + variant v; try { v.emplace<1>(A{}); -- 2.30.2