From 8d6922ae46e87618ed58579f977d5766332c377c Mon Sep 17 00:00:00 2001 From: achabense <60953653+achabense@users.noreply.github.com> Date: Sun, 15 Oct 2023 02:51:05 +0800 Subject: [PATCH] Lifetime cleanups for `basic_string` (#4047) Co-authored-by: Stephan T. Lavavej --- stl/inc/xstring | 91 +++++++++++-------- .../test.cpp | 21 +++++ 2 files changed, 75 insertions(+), 37 deletions(-) diff --git a/stl/inc/xstring b/stl/inc/xstring index 8b7fc9485..26cc28565 100644 --- a/stl/inc/xstring +++ b/stl/inc/xstring @@ -2287,6 +2287,19 @@ public: value_type _Buf[_BUF_SIZE]; pointer _Ptr; char _Alias[_BUF_SIZE]; // TRANSITION, ABI: _Alias is preserved for binary compatibility (especially /clr) + + _CONSTEXPR20 void _Switch_to_buf() noexcept { + _STD _Destroy_in_place(_Ptr); + +#if _HAS_CXX20 + // start the lifetime of the array elements + if (_STD is_constant_evaluated()) { + for (size_type _Idx = 0; _Idx < _BUF_SIZE; ++_Idx) { + _Buf[_Idx] = value_type(); + } + } +#endif // _HAS_CXX20 + } }; _Bxty _Bx; @@ -2484,13 +2497,11 @@ private: public: _CONSTEXPR20 basic_string() noexcept(is_nothrow_default_constructible_v<_Alty>) : _Mypair(_Zero_then_variadic_args_t{}) { - _Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal())); - _Tidy_init(); + _Construct_empty(); } _CONSTEXPR20 explicit basic_string(const _Alloc& _Al) noexcept : _Mypair(_One_then_variadic_args_t{}, _Al) { - _Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal())); - _Tidy_init(); + _Construct_empty(); } _CONSTEXPR20 basic_string(const basic_string& _Right) @@ -2578,8 +2589,7 @@ public: auto _UFirst = _Get_unwrapped(_First); auto _ULast = _Get_unwrapped(_Last); if (_UFirst == _ULast) { - _Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal())); - _Tidy_init(); + _Construct_empty(); } else { if constexpr (_Is_elem_cptr::value) { _Construct<_Construct_strategy::_From_ptr>( @@ -2630,6 +2640,19 @@ private: _Al.deallocate(_Old_ptr, _Capacity + 1); // +1 for null terminator } + _CONSTEXPR20 void _Construct_empty() { + auto& _My_data = _Mypair._Myval2; + _My_data._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal())); + + // initialize basic_string data members + _My_data._Mysize = 0; + _My_data._Myres = _Small_string_capacity; + _My_data._Activate_SSO_buffer(); + + // the _Traits::assign is last so the codegen doesn't think the char write can alias this + _Traits::assign(_My_data._Bx._Buf[0], _Elem()); + } + enum class _Construct_strategy : uint8_t { _From_char, _From_ptr, _From_string }; template <_Construct_strategy _Strat, class _Char_or_ptr> @@ -2918,8 +2941,7 @@ public: basic_string(_String_constructor_rvalue_allocator_tag, _Alloc&& _Al) : _Mypair(_One_then_variadic_args_t{}, _STD move(_Al)) { // Used exclusively by basic_stringbuf - _Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Getal())); - _Tidy_init(); + _Construct_empty(); } _NODISCARD bool _Move_assign_from_buffer( @@ -2958,10 +2980,13 @@ public: _Released_buffer _Result; auto& _My_data = _Mypair._Myval2; _Result._Size = _My_data._Mysize; + _My_data._Orphan_all(); _ASAN_STRING_REMOVE(*this); if (_My_data._Large_mode_engaged()) { _Result._Ptr = _My_data._Bx._Ptr; _Result._Actual_allocation_size = _My_data._Myres + 1; + + _My_data._Bx._Switch_to_buf(); } else { // use _Least_allocation_size to avoid small mode, if the buffer is assigned back size_type _Allocated = _Least_allocation_size; @@ -2969,8 +2994,9 @@ public: _Traits::copy(_Unfancy(_Result._Ptr), _My_data._Bx._Buf, _BUF_SIZE); _Result._Actual_allocation_size = _Allocated; } - _My_data._Orphan_all(); - _Tidy_init(); + _My_data._Mysize = 0; + _My_data._Myres = _Small_string_capacity; + _Traits::assign(_My_data._Bx._Buf[0], _Elem()); return _Result; } #endif // _HAS_CXX20 @@ -3038,27 +3064,34 @@ private: const auto _Right_data_mem = reinterpret_cast(_STD addressof(_Right._Mypair._Myval2)) + _Memcpy_val_offset; _CSTD memcpy(_My_data_mem, _Right_data_mem, _Memcpy_val_size); - _Right._Tidy_init(); + + _Right_data._Mysize = 0; + _Right_data._Myres = _Small_string_capacity; + _Right_data._Activate_SSO_buffer(); + _Traits::assign(_Right_data._Bx._Buf[0], _Elem()); return; } } #endif // !defined(_INSERT_STRING_ANNOTATION) if (_Right_data._Large_mode_engaged()) { // steal buffer - _Construct_in_place(_My_data._Bx._Ptr, _Right_data._Bx._Ptr); _Swap_proxy_and_iterators(_Right); - _Destroy_in_place(_Right_data._Bx._Ptr); + _Construct_in_place(_My_data._Bx._Ptr, _Right_data._Bx._Ptr); + _Right_data._Bx._Switch_to_buf(); } else { // copy small string buffer + _Right_data._Orphan_all(); + _My_data._Activate_SSO_buffer(); _Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1); - _Right_data._Orphan_all(); } _My_data._Myres = _Right_data._Myres; _My_data._Mysize = _Right_data._Mysize; - _Right._Tidy_init(); + _Right_data._Mysize = 0; + _Right_data._Myres = _Small_string_capacity; + _Traits::assign(_Right_data._Bx._Buf[0], _Elem()); } #if _HAS_CXX23 @@ -4252,9 +4285,7 @@ public: static _CONSTEXPR20 void _Swap_bx_large_with_small(_Scary_val& _Starts_large, _Scary_val& _Starts_small) noexcept { // exchange a string in large mode with one in small mode const pointer _Ptr = _Starts_large._Bx._Ptr; - _Destroy_in_place(_Starts_large._Bx._Ptr); - - _Starts_large._Activate_SSO_buffer(); + _Starts_large._Bx._Switch_to_buf(); _Traits::copy(_Starts_large._Bx._Buf, _Starts_small._Bx._Buf, _BUF_SIZE); _Construct_in_place(_Starts_small._Bx._Ptr, _Ptr); @@ -4802,10 +4833,9 @@ private: _My_data._Orphan_all(); _ASAN_STRING_REMOVE(*this); const pointer _Ptr = _My_data._Bx._Ptr; - auto& _Al = _Getal(); - _Destroy_in_place(_My_data._Bx._Ptr); - _My_data._Activate_SSO_buffer(); + _My_data._Bx._Switch_to_buf(); _Traits::copy(_My_data._Bx._Buf, _Unfancy(_Ptr), _My_data._Mysize + 1); + auto& _Al = _Getal(); _Deallocate_for_capacity(_Al, _Ptr, _My_data._Myres); _My_data._Myres = _Small_string_capacity; } @@ -4815,27 +4845,14 @@ private: _Traits::assign(_Mypair._Myval2._Myptr()[_Mypair._Myval2._Mysize = _New_size], _Elem()); } - _CONSTEXPR20 void _Tidy_init() noexcept { - // initialize basic_string data members - auto& _My_data = _Mypair._Myval2; - _My_data._Mysize = 0; - _My_data._Myres = _Small_string_capacity; - _My_data._Activate_SSO_buffer(); - - // the _Traits::assign is last so the codegen doesn't think the char write can alias this - _Traits::assign(_My_data._Bx._Buf[0], _Elem()); - } - _CONSTEXPR20 void _Tidy_deallocate() noexcept { // initialize buffer, deallocating any storage auto& _My_data = _Mypair._Myval2; _My_data._Orphan_all(); if (_My_data._Large_mode_engaged()) { _ASAN_STRING_REMOVE(*this); - const pointer _Ptr = _My_data._Bx._Ptr; - auto& _Al = _Getal(); - _Destroy_in_place(_My_data._Bx._Ptr); - _My_data._Activate_SSO_buffer(); - _Deallocate_for_capacity(_Al, _Ptr, _My_data._Myres); + auto& _Al = _Getal(); + _Deallocate_for_capacity(_Al, _My_data._Bx._Ptr, _My_data._Myres); + _My_data._Bx._Switch_to_buf(); } _My_data._Mysize = 0; diff --git a/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp b/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp index 5d9410708..c07d9a658 100644 --- a/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp +++ b/tests/std/tests/VSO_0000000_allocator_propagation/test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -971,6 +973,20 @@ _CONSTEXPR20 void test_string_swap(const size_t id1, const size_t id2) { assert(dst.get_allocator().id() == id1); } +#if _HAS_CXX20 +void test_string_move_to_stringbuf() { + // GH-4047 fixed a bug where basic_string forgets to destroy the pointer before switching to small + // mode. This will turn problematic if the pointer is non-trivial. + assert(!_CrtDumpMemoryLeaks()); + { + using Alloc = StationaryAlloc; + basic_string, Alloc> str(50, '0', Alloc(10)); + basic_stringbuf, Alloc> strbuf(move(str)); + } + assert(!_CrtDumpMemoryLeaks()); +} +#endif // _HAS_CXX20 + _CONSTEXPR20 bool test_string() { test_string_copy_ctor(); @@ -1002,6 +1018,11 @@ _CONSTEXPR20 bool test_string() { test_string_swap>(11, 22); // POCS, non-equal allocators test_string_swap>(11, 22); // POCS, always-equal allocators +#if _HAS_CXX20 + if (!is_constant_evaluated()) { + test_string_move_to_stringbuf(); + } +#endif // _HAS_CXX20 return true; }