From 1b3f9ba2eb4a5f10fd62732f478900c2f899b3e3 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Mon, 9 Jan 2023 00:07:56 -0800 Subject: [PATCH 1/3] : constexpr string element lifetime I've never trusted our use of `_Traits::assign` to start the lifetimes of the elements given that, err, that's not what `_Traits::assign` does. Let's use a `construct_at` loop instead, and centralize it into a helper function instead of repeating nine times. Test coverage will be in a future LLVM update. Drive-by: Use `_Traits::copy` instead of `_Traits::move` in `_Construct`. Since we're constructing a new string, we now the argument doesn't alias our content. --- stl/inc/xstring | 74 +++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 48 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index d159988e4ff..7879d6fe813 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2629,6 +2629,20 @@ public: } private: + static constexpr void _Start_element_lifetimes(_Elem* const _Ptr, const size_type _Size) { +#if _HAS_CXX20 + if (_STD is_constant_evaluated()) { + // Start element lifetimes to avoid UB + for (size_type _Idx = 0; _Idx < _Size; ++_Idx) { + _STD construct_at(_Ptr + _Idx); + } + } +#else + (void) _Ptr; + (void) _Size; +#endif // _HAS_CXX20 + } + enum class _Construct_strategy : uint8_t { _From_char, _From_ptr, _From_string }; template <_Construct_strategy _Strat, class _Char_or_ptr> @@ -2658,13 +2672,13 @@ private: _Traits::assign(_My_data._Bx._Buf, _Count, _Arg); _Traits::assign(_My_data._Bx._Buf[_Count], _Elem()); } else if constexpr (_Strat == _Construct_strategy::_From_ptr) { - _Traits::move(_My_data._Bx._Buf, _Arg, _Count); + _Traits::copy(_My_data._Bx._Buf, _Arg, _Count); _Traits::assign(_My_data._Bx._Buf[_Count], _Elem()); } else { // _Strat == _Construct_strategy::_From_string #ifdef _INSERT_STRING_ANNOTATION - _Traits::move(_My_data._Bx._Buf, _Arg, _Count + 1); + _Traits::copy(_My_data._Bx._Buf, _Arg, _Count + 1); #else // ^^^ _INSERT_STRING_ANNOTATION / !_INSERT_STRING_ANNOTATION vvv - _Traits::move(_My_data._Bx._Buf, _Arg, _BUF_SIZE); + _Traits::copy(_My_data._Bx._Buf, _Arg, _BUF_SIZE); #endif // !_INSERT_STRING_ANNOTATION } @@ -2677,11 +2691,7 @@ private: const pointer _New_ptr = _Al.allocate(_New_capacity + 1); // throws _Construct_in_place(_My_data._Bx._Ptr, _New_ptr); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_capacity + 1); _My_data._Mysize = _Count; _My_data._Myres = _New_capacity; @@ -2725,11 +2735,7 @@ private: _Construct_in_place(_My_data._Bx._Ptr, _New_ptr); _My_data._Myres = _New_capacity; -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_capacity + 1); } } @@ -2745,11 +2751,7 @@ private: const size_type _New_capacity = _Calculate_growth(_My_data._Mysize); const pointer _New_ptr = _Al.allocate(_New_capacity + 1); // throws -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_capacity + 1); _Traits::copy(_Unfancy(_New_ptr), _Old_ptr, _My_data._Mysize); if (_My_data._Myres >= _BUF_SIZE) { // Need to deallocate old storage _Al.deallocate(_My_data._Bx._Ptr, _My_data._Myres + 1); @@ -2834,11 +2836,7 @@ public: _Ptr = _Unfancy(_Fancyptr); _Construct_in_place(_My_data._Bx._Ptr, _Fancyptr); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Ptr, _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Ptr, _New_capacity + 1); } _My_data._Mysize = _New_size; @@ -2909,11 +2907,7 @@ public: _Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _My_data); // throws const pointer _Fancyptr = _Getal().allocate(_New_capacity + 1); // throws // nothrow hereafter -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_Fancyptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_Fancyptr), _New_capacity + 1); _Construct_in_place(_My_data._Bx._Ptr, _Fancyptr); _My_data._Mysize = _New_size; _My_data._Myres = _New_capacity; @@ -3217,11 +3211,7 @@ public: auto _Right_al_non_const = _Right_al; const auto _New_ptr = _Right_al_non_const.allocate(_New_capacity + 1); // throws -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_size + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_size + 1); _Traits::copy(_Unfancy(_New_ptr), _Unfancy(_Right._Mypair._Myval2._Bx._Ptr), _New_size + 1); _Tidy_deallocate(); @@ -4049,11 +4039,7 @@ public: const pointer _New_ptr = _Al.allocate(_Target_capacity + 1); // throws _ASAN_STRING_REMOVE(*this); -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _Target_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _Target_capacity + 1); _My_data._Orphan_all(); _Traits::copy(_Unfancy(_New_ptr), _Unfancy(_My_data._Bx._Ptr), _My_data._Mysize + 1); @@ -4792,11 +4778,7 @@ private: auto& _Al = _Getal(); const pointer _New_ptr = _Al.allocate(_New_capacity + 1); // throws -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_capacity + 1); _Mypair._Myval2._Orphan_all(); _ASAN_STRING_REMOVE(*this); _Mypair._Myval2._Mysize = _New_size; @@ -4829,11 +4811,7 @@ private: auto& _Al = _Getal(); const pointer _New_ptr = _Al.allocate(_New_capacity + 1); // throws -#if _HAS_CXX20 - if (_STD is_constant_evaluated()) { // Begin the lifetimes of the objects before copying to avoid UB - _Traits::assign(_Unfancy(_New_ptr), _New_capacity + 1, _Elem()); - } -#endif // _HAS_CXX20 + _Start_element_lifetimes(_Unfancy(_New_ptr), _New_capacity + 1); _My_data._Orphan_all(); _ASAN_STRING_REMOVE(*this); _My_data._Mysize = _New_size; From 8650981d5820e9685e81ae2db67bb7c8d80233a8 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Mon, 9 Jan 2023 02:11:51 -0800 Subject: [PATCH 2/3] Fixed seven libcxx skips! --- tests/libcxx/expected_results.txt | 7 ------- tests/libcxx/skipped_tests.txt | 7 ------- 2 files changed, 14 deletions(-) diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt index a77a6c9724a..d1d5b5ed10f 100644 --- a/tests/libcxx/expected_results.txt +++ b/tests/libcxx/expected_results.txt @@ -1009,7 +1009,6 @@ std/ranges/range.adaptors/range.lazy.split/view_interface.pass.cpp FAIL std/ranges/range.adaptors/range.take/adaptor.pass.cpp FAIL std/ranges/range.factories/range.single.view/cpo.pass.cpp FAIL std/strings/basic.string/string.cons/dtor.pass.cpp FAIL -std/strings/basic.string/string.cons/implicit_deduction_guides.pass.cpp FAIL std/strings/basic.string/string.modifiers/string_erase/iter.pass.cpp:0 FAIL std/strings/basic.string/string.modifiers/string_erase/iter_iter.pass.cpp:0 FAIL std/strings/basic.string/string.modifiers/string_insert/iter_iter_iter.pass.cpp:0 FAIL @@ -1020,12 +1019,6 @@ std/strings/basic.string/string.modifiers/string_replace/iter_iter_pointer_size. std/strings/basic.string/string.modifiers/string_replace/iter_iter_size_char.pass.cpp:0 FAIL std/strings/basic.string/string.modifiers/string_replace/iter_iter_string.pass.cpp:0 FAIL std/strings/basic.string/string.modifiers/string_replace/iter_iter_string_view.pass.cpp:0 FAIL -std/strings/string.view/string.view.comparison/equal.pass.cpp FAIL -std/strings/string.view/string.view.comparison/greater.pass.cpp FAIL -std/strings/string.view/string.view.comparison/greater_equal.pass.cpp FAIL -std/strings/string.view/string.view.comparison/less.pass.cpp FAIL -std/strings/string.view/string.view.comparison/less_equal.pass.cpp FAIL -std/strings/string.view/string.view.comparison/not_equal.pass.cpp FAIL std/thread/futures/futures.task/futures.task.members/ctor2.compile.pass.cpp FAIL std/utilities/function.objects/func.wrap/func.wrap.func/addressof.pass.cpp:0 FAIL std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.alg/swap.pass.cpp:0 FAIL diff --git a/tests/libcxx/skipped_tests.txt b/tests/libcxx/skipped_tests.txt index fdcf3360ad3..ea976d99489 100644 --- a/tests/libcxx/skipped_tests.txt +++ b/tests/libcxx/skipped_tests.txt @@ -1009,7 +1009,6 @@ ranges\range.adaptors\range.lazy.split\view_interface.pass.cpp ranges\range.adaptors\range.take\adaptor.pass.cpp ranges\range.factories\range.single.view\cpo.pass.cpp strings\basic.string\string.cons\dtor.pass.cpp -strings\basic.string\string.cons\implicit_deduction_guides.pass.cpp strings\basic.string\string.modifiers\string_erase\iter.pass.cpp strings\basic.string\string.modifiers\string_erase\iter_iter.pass.cpp strings\basic.string\string.modifiers\string_insert\iter_iter_iter.pass.cpp @@ -1020,12 +1019,6 @@ strings\basic.string\string.modifiers\string_replace\iter_iter_pointer_size.pass strings\basic.string\string.modifiers\string_replace\iter_iter_size_char.pass.cpp strings\basic.string\string.modifiers\string_replace\iter_iter_string.pass.cpp strings\basic.string\string.modifiers\string_replace\iter_iter_string_view.pass.cpp -strings\string.view\string.view.comparison\equal.pass.cpp -strings\string.view\string.view.comparison\greater.pass.cpp -strings\string.view\string.view.comparison\greater_equal.pass.cpp -strings\string.view\string.view.comparison\less.pass.cpp -strings\string.view\string.view.comparison\less_equal.pass.cpp -strings\string.view\string.view.comparison\not_equal.pass.cpp thread\futures\futures.task\futures.task.members\ctor2.compile.pass.cpp utilities\function.objects\func.wrap\func.wrap.func\addressof.pass.cpp utilities\function.objects\func.wrap\func.wrap.func\func.wrap.func.alg\swap.pass.cpp From f609917bd0ff05ef8d65addc2b623b47378c3c37 Mon Sep 17 00:00:00 2001 From: Casey Carter Date: Tue, 10 Jan 2023 13:53:15 -0800 Subject: [PATCH 3/3] Clarifying comments Address STL's review comment, and clarify the difference between _Activate_SSO_buffer and _Start_element_lifetimes. --- stl/inc/xstring | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 7879d6fe813..23476903226 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2286,7 +2286,7 @@ public: } constexpr void _Activate_SSO_buffer() noexcept { - // begin the lifetime of the array elements (e.g., before copying into them) + // start the lifetime of the array elements #if _HAS_CXX20 if (_STD is_constant_evaluated()) { for (size_type _Idx = 0; _Idx < _BUF_SIZE; ++_Idx) { @@ -2630,14 +2630,15 @@ public: private: static constexpr void _Start_element_lifetimes(_Elem* const _Ptr, const size_type _Size) { + // Start element lifetimes to avoid UB. This is a more general mechanism than _String_val::_Activate_SSO_buffer, + // but likely more impactful to throughput. #if _HAS_CXX20 if (_STD is_constant_evaluated()) { - // Start element lifetimes to avoid UB for (size_type _Idx = 0; _Idx < _Size; ++_Idx) { _STD construct_at(_Ptr + _Idx); } } -#else +#else // ^^^ C++20-or-later / pre-C++20 vvv (void) _Ptr; (void) _Size; #endif // _HAS_CXX20