Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix the _Choose_pocca_v branch in basic_string::operator=(const&) #4031

Merged
merged 12 commits into from
Sep 21, 2023
Merged

Fix the _Choose_pocca_v branch in basic_string::operator=(const&) #4031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
36de4e9
Add test coverage for non-trivial pointers
achabense Sep 17, 2023
af25008
Fix `basic_string::operator=(const&)`
achabense Sep 18, 2023
02b66e9
Fix `basic_string::_Take_contents`
achabense Sep 18, 2023
3cb5d00
[WIP] `failure was caused by allocated storage not being deallocated`…
achabense Sep 18, 2023
add808d
Update `operator bool()`
achabense Sep 19, 2023
15e6762
cleanups
achabense Sep 19, 2023
d724676
Add `private:`.
StephanTLavavej Sep 19, 2023
7f3ba48
`_CONSTEXPR20 friend` => `friend _CONSTEXPR20`
StephanTLavavej Sep 19, 2023
33dce61
Use the primary template for when `is_void_v` is `true`.
StephanTLavavej Sep 19, 2023
9daeca5
`struct` defaults to `public` inheritance.
StephanTLavavej Sep 19, 2023
4a51b97
Work around VSO-1888462.
StephanTLavavej Sep 19, 2023
a6795f4
Restrict the MSVC workaround to C++20 compile-time.
StephanTLavavej Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions stl/inc/xstring
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3052,8 +3052,9 @@ private:

if (_Right_data._Large_mode_engaged()) { // steal buffer
_Construct_in_place(_My_data._Bx._Ptr, _Right_data._Bx._Ptr);
_Right_data._Bx._Ptr = nullptr;
Copy link
Contributor Author

@achabense achabense Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emm, for clarity it might be better just to put the correction _Destroy_in_place(_Right_data._Bx._Ptr); at the original place.

Update: (Not planned in this pr)

_Destroy_in_place is typically followed by _Activate_SSO_buffer; however in this function, the _Activate_SSO_buffer call is put off to _Right._Tidy_init(); at the end. This suggests highly flaky maintenance state. I think we need to re-investigate the usages of _Activate_SSO_buffer / _Tidy_init / ... in the codebase.

Especially, I believe there is no need to call _Activate_SSO_buffer at basic_string's constructor, as the _String_val already initialized the buffer in its ctor. However, a lot of basic_string's ctor calls a _Tidy_init, which includes a _Activate_SSO_buffer.

STL/stl/inc/xstring

Line 2284 in 8f67ece

_CONSTEXPR20 _Bxty() noexcept : _Buf() {} // user-provided, for fancy pointers

_Destroy_in_place is closely related to _Activate_SSO_buffer. When switching to SSO mode both function must be called in order. However there are several places that the they are dangerously separated into different function calls. Especially, I've found another place that lacks _Destroy_in_place. (in next comment)
In short, I think we should do _Activate_SSO_buffer and _Destroy_in_place only when switching back to SSO mode, and the two functions are so closely related that they should not be separated into different functions. As this pr shows, the separation can result in maintenance burden and has become a source of bugs.

_Swap_proxy_and_iterators(_Right);

_Destroy_in_place(_Right_data._Bx._Ptr);
Copy link
Contributor Author

@achabense achabense Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Tidy_init assumes that the pointer is already deactivated. (Perhaps we need more cleanups for the relevant logics in the future.)

} else { // copy small string buffer
_My_data._Activate_SSO_buffer();
_Traits::copy(_My_data._Bx._Buf, _Right_data._Bx._Buf, _Right_data._Mysize + 1);
Expand Down Expand Up @@ -3166,12 +3167,13 @@ public:
size_type _New_capacity = _Calculate_growth(_Right_size, _Small_string_capacity, _Right.max_size());
auto _Right_al_non_const = _Right_al;
const pointer _New_ptr = _Allocate_for_capacity(_Right_al_non_const, _New_capacity); // throws

_Traits::copy(_Unfancy(_New_ptr), _Right_ptr, _Right_size + 1);

_Tidy_deallocate();
_Mypair._Myval2._Bx._Ptr = _New_ptr;
_Mypair._Myval2._Mysize = _Right_size;
_Mypair._Myval2._Myres = _New_capacity;
_Construct_in_place(_Mypair._Myval2._Bx._Ptr, _New_ptr);
_Mypair._Myval2._Mysize = _Right_size;
_Mypair._Myval2._Myres = _New_capacity;
_ASAN_STRING_CREATE(*this);
} else {
_Tidy_deallocate();
_Traits::copy(_Mypair._Myval2._Bx._Buf, _Right_ptr, _Right_size + 1);
Expand Down
193 changes: 179 additions & 14 deletions tests/std/tests/VSO_0000000_allocator_propagation/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,160 @@ _CONSTEXPR20 void assert_is_permutation(const Container& cont, initializer_list<
assert(is_permutation(cont.begin(), cont.end(), il.begin(), il.end()));
}

class test_leak {
private:
char* res;

public:
test_leak(const test_leak&) = delete;
test_leak& operator=(const test_leak&) = delete;

_CONSTEXPR20 test_leak() noexcept /* terminates */ : res(allocator<char>{}.allocate(1)) {}
_CONSTEXPR20 ~test_leak() {
allocator<char>{}.deallocate(res, 1);
}
};

template <class T>
class nontrivial_pointer : private test_leak {
public:
T* ptr;
_CONSTEXPR20 nontrivial_pointer() noexcept : ptr(nullptr) {}
_CONSTEXPR20 explicit nontrivial_pointer(T* ptr_) noexcept : ptr(ptr_) {}
_CONSTEXPR20 /*implicit*/ nontrivial_pointer(nullptr_t) noexcept : ptr(nullptr) {}

_CONSTEXPR20 nontrivial_pointer(const nontrivial_pointer& other) noexcept : ptr(other.ptr) {}
template <class U, enable_if_t<is_convertible_v<U*, T*>, int> = 0>
_CONSTEXPR20 nontrivial_pointer(const nontrivial_pointer<U>& other) noexcept : ptr(other.ptr) {}
_CONSTEXPR20 nontrivial_pointer& operator=(const nontrivial_pointer& other) noexcept {
ptr = other.ptr;
return *this;
}

_CONSTEXPR20 ~nontrivial_pointer() = default;

_CONSTEXPR20 explicit operator bool() const noexcept {
#ifdef __EDG__ // TRANSITION, VSO-1888157
return static_cast<bool>(ptr);
achabense marked this conversation as resolved.
Show resolved Hide resolved
#else // ^^^ workaround / no workaround vvv
return ptr != nullptr;
#endif // ^^^ no workaround ^^^
}
_CONSTEXPR20 add_lvalue_reference_t<T> operator*() const noexcept {
return *ptr;
}
_CONSTEXPR20 T* operator->() const noexcept {
return ptr;
}
template <class I, enable_if_t<is_integral_v<I>, int> = 0>
_CONSTEXPR20 add_lvalue_reference_t<T> operator[](I off) const noexcept {
return ptr[off];
}

_CONSTEXPR20 nontrivial_pointer& operator++() noexcept {
++ptr;
return *this;
}
_CONSTEXPR20 nontrivial_pointer operator++(int) noexcept {
return nontrivial_pointer(ptr++);
}
_CONSTEXPR20 nontrivial_pointer& operator--() noexcept {
--ptr;
return *this;
}
_CONSTEXPR20 nontrivial_pointer operator--(int) noexcept {
return nontrivial_pointer(ptr--);
}
template <class I, enable_if_t<is_integral_v<I>, int> = 0>
_CONSTEXPR20 nontrivial_pointer& operator+=(I diff) noexcept {
ptr += diff;
return *this;
}
template <class I, enable_if_t<is_integral_v<I>, int> = 0>
_CONSTEXPR20 nontrivial_pointer& operator-=(I diff) noexcept {
ptr -= diff;
return *this;
}

template <class I, enable_if_t<is_integral_v<I>, int> = 0>
friend _CONSTEXPR20 nontrivial_pointer operator+(nontrivial_pointer ptr, I diff) noexcept {
return ptr += diff;
}
template <class I, enable_if_t<is_integral_v<I>, int> = 0>
friend _CONSTEXPR20 nontrivial_pointer operator+(I diff, nontrivial_pointer ptr) noexcept {
return ptr += diff;
}
friend _CONSTEXPR20 ptrdiff_t operator-(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr - rhs.ptr;
}
template <class I, enable_if_t<is_integral_v<I>, int> = 0>
friend _CONSTEXPR20 nontrivial_pointer operator-(nontrivial_pointer ptr, I diff) noexcept {
return ptr -= diff;
}

friend _CONSTEXPR20 bool operator==(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr == rhs.ptr;
}
friend _CONSTEXPR20 bool operator==(nontrivial_pointer ptr, nullptr_t) noexcept {
return !ptr;
}
friend _CONSTEXPR20 bool operator==(nullptr_t, nontrivial_pointer ptr) noexcept {
return !ptr;
}
friend _CONSTEXPR20 bool operator!=(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr != rhs.ptr;
}
friend _CONSTEXPR20 bool operator!=(nontrivial_pointer ptr, nullptr_t) noexcept {
return static_cast<bool>(ptr);
}
friend _CONSTEXPR20 bool operator!=(nullptr_t, nontrivial_pointer ptr) noexcept {
return static_cast<bool>(ptr);
}
friend _CONSTEXPR20 bool operator<(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr < rhs.ptr;
}
friend _CONSTEXPR20 bool operator<=(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr <= rhs.ptr;
}
friend _CONSTEXPR20 bool operator>(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr > rhs.ptr;
}
friend _CONSTEXPR20 bool operator>=(nontrivial_pointer lhs, nontrivial_pointer rhs) noexcept {
return lhs.ptr >= rhs.ptr;
}
};

template <class T, bool IsVoid = is_void_v<T>>
struct impl_pointer_to {
// no pointer_to for void
};

template <class T>
struct impl_pointer_to<T, false> {
static constexpr nontrivial_pointer<T> pointer_to(T& r) noexcept {
return nontrivial_pointer<T>(addressof(r));
}
};

template <class T>
struct std::pointer_traits<nontrivial_pointer<T>> : impl_pointer_to<T> {
using pointer = nontrivial_pointer<T>;
using element_type = T;
using difference_type = ptrdiff_t;

template <class U>
using rebind = nontrivial_pointer<U>;
};

template <class T>
struct std::iterator_traits<nontrivial_pointer<T>> {
using iterator_category = random_access_iterator_tag;
using difference_type = ptrdiff_t;
using value_type = remove_const_t<T>;
using reference = add_lvalue_reference_t<T>;
using pointer = nontrivial_pointer<T>;
};

template <class T, class POCCA, class POCMA, class POCS, class EQUAL>
class MyAlloc {
private:
Expand All @@ -49,6 +203,7 @@ class MyAlloc {
}

using value_type = T;
using pointer = nontrivial_pointer<T>;

using propagate_on_container_copy_assignment = POCCA;
using propagate_on_container_move_assignment = POCMA;
Expand All @@ -70,12 +225,12 @@ class MyAlloc {
return equal_id() != other.equal_id();
}

[[nodiscard]] constexpr T* allocate(const size_t numElements) {
return allocator<T>{}.allocate(numElements + equal_id()) + equal_id();
[[nodiscard]] constexpr pointer allocate(const size_t numElements) {
return pointer(allocator<T>{}.allocate(numElements + equal_id()) + equal_id());
}

constexpr void deallocate(T* const first, const size_t numElements) noexcept {
allocator<T>{}.deallocate(first - equal_id(), numElements + equal_id());
constexpr void deallocate(pointer const first, const size_t numElements) noexcept {
allocator<T>{}.deallocate(_Unfancy(first - equal_id()), numElements + equal_id());
}
};

Expand Down Expand Up @@ -307,7 +462,12 @@ _CONSTEXPR20 bool test_sequence() {
test_sequence_copy_assign<Sequence, CopyAlloc<int>>(11, 22, 11); // POCCA, non-equal allocators
test_sequence_copy_assign<Sequence, CopyEqualAlloc<int>>(11, 22, 11); // POCCA, always-equal allocators

test_sequence_move_ctor<Sequence>();
#if _HAS_CXX20 && !defined(__clang__) && !defined(__EDG__) // TRANSITION, VSO-1888462
if (!is_constant_evaluated())
#endif // ^^^ workaround ^^^
{
test_sequence_move_ctor<Sequence>();
}

test_sequence_move_alloc_ctor<Sequence>(11, 11); // equal allocators
test_sequence_move_alloc_ctor<Sequence>(11, 22); // non-equal allocators
Expand Down Expand Up @@ -559,7 +719,7 @@ void test_flist() {

// NOTE: Having 4 elements of type char32_t bypasses the Small String Optimization.

void test_string_copy_ctor() {
_CONSTEXPR20 void test_string_copy_ctor() {
basic_string<char32_t, char_traits<char32_t>, StationaryAlloc<char32_t>> src(
{5, 10, 20, 30}, StationaryAlloc<char32_t>(11));
auto src_it = src.begin();
Expand All @@ -583,7 +743,7 @@ void test_string_copy_ctor() {
assert(dst.get_allocator().id() == 11);
}

void test_string_copy_alloc_ctor(const size_t id1, const size_t id2) {
_CONSTEXPR20 void test_string_copy_alloc_ctor(const size_t id1, const size_t id2) {
basic_string<char32_t, char_traits<char32_t>, StationaryAlloc<char32_t>> src(
{5, 10, 20, 30}, StationaryAlloc<char32_t>(id1));
auto src_it = src.begin();
Expand All @@ -608,7 +768,7 @@ void test_string_copy_alloc_ctor(const size_t id1, const size_t id2) {
}

template <class Alloc>
void test_string_copy_assign(const size_t id1, const size_t id2, const size_t id3) {
_CONSTEXPR20 void test_string_copy_assign(const size_t id1, const size_t id2, const size_t id3) {

basic_string<char32_t, char_traits<char32_t>, Alloc> src({5, 10, 20, 30}, Alloc(id1));
basic_string<char32_t, char_traits<char32_t>, Alloc> dst({0, 0, 0, 0}, Alloc(id2));
Expand Down Expand Up @@ -636,7 +796,7 @@ void test_string_copy_assign(const size_t id1, const size_t id2, const size_t id
assert(dst.get_allocator().id() == id3);
}

void test_string_copy_assign_pocca_sso() {
_CONSTEXPR20 void test_string_copy_assign_pocca_sso() {
// GH-3862 fixed a bug where the POCCA codepath in basic_string's copy assignment operator mishandled
// the scenario where the string on the right hand side has a large capacity but a small size - so while
// the RHS has dynamically allocated memory, the LHS should activate the Small String Optimization.
Expand All @@ -661,7 +821,7 @@ void test_string_copy_assign_pocca_sso() {
assert(right == "yyyyyyy");
}

void test_string_move_ctor() {
_CONSTEXPR20 void test_string_move_ctor() {
basic_string<char32_t, char_traits<char32_t>, StationaryAlloc<char32_t>> src(
{5, 10, 20, 30}, StationaryAlloc<char32_t>(11));
auto it1 = src.begin();
Expand Down Expand Up @@ -690,7 +850,7 @@ void test_string_move_ctor() {
assert(dst.get_allocator().id() == 11);
}

void test_string_move_alloc_ctor(const size_t id1, const size_t id2) {
_CONSTEXPR20 void test_string_move_alloc_ctor(const size_t id1, const size_t id2) {
basic_string<char32_t, char_traits<char32_t>, StationaryAlloc<char32_t>> src(
{5, 10, 20, 30}, StationaryAlloc<char32_t>(id1));
auto it1 = src.begin();
Expand Down Expand Up @@ -721,7 +881,7 @@ void test_string_move_alloc_ctor(const size_t id1, const size_t id2) {
}

template <class Alloc>
void test_string_move_assign(const size_t id1, const size_t id2, const size_t id3) {
_CONSTEXPR20 void test_string_move_assign(const size_t id1, const size_t id2, const size_t id3) {

basic_string<char32_t, char_traits<char32_t>, Alloc> src({5, 10, 20, 30}, Alloc(id1));
basic_string<char32_t, char_traits<char32_t>, Alloc> dst({0, 0, 0, 0}, Alloc(id2));
Expand Down Expand Up @@ -755,7 +915,7 @@ void test_string_move_assign(const size_t id1, const size_t id2, const size_t id
}

template <class Alloc>
void test_string_swap(const size_t id1, const size_t id2) {
_CONSTEXPR20 void test_string_swap(const size_t id1, const size_t id2) {

basic_string<char32_t, char_traits<char32_t>, Alloc> src({5, 10, 20, 30}, Alloc(id1));
basic_string<char32_t, char_traits<char32_t>, Alloc> dst({6, 40, 50, 60}, Alloc(id2));
Expand Down Expand Up @@ -784,7 +944,7 @@ void test_string_swap(const size_t id1, const size_t id2) {
assert(dst.get_allocator().id() == id1);
}

void test_string() {
_CONSTEXPR20 bool test_string() {
test_string_copy_ctor();

test_string_copy_alloc_ctor(11, 11); // equal allocators
Expand Down Expand Up @@ -814,6 +974,8 @@ void test_string() {
test_string_swap<SwapAlloc<char32_t>>(11, 11); // POCS, equal allocators
test_string_swap<SwapAlloc<char32_t>>(11, 22); // POCS, non-equal allocators
test_string_swap<SwapEqualAlloc<char32_t>>(11, 22); // POCS, always-equal allocators

return true;
}


Expand Down Expand Up @@ -1680,8 +1842,11 @@ int main() {
test_sequence<deque>();
test_sequence<list>();
test_sequence<vector>();

#if _HAS_CXX20
static_assert(test_sequence<vector>());

static_assert(test_string());
#endif // _HAS_CXX20

test_flist();
Expand Down