[libcxx-commits] [libcxx] [libc++] Fix shrink_to_fit to swap buffer only when capacity is strictly smaller (PR #127321)
Peng Liu via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Feb 19 14:19:48 PST 2025
https://github.com/winner245 updated https://github.com/llvm/llvm-project/pull/127321
>From 0b54927a5fcd8f335f7ebb24ba584a6559158cea Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Sat, 15 Feb 2025 08:44:51 -0500
Subject: [PATCH 1/5] Fix shrink_to_fit to swap buffer only when capacity is
strictly smaller
---
libcxx/include/string | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libcxx/include/string b/libcxx/include/string
index 3f43e8fd8d586..fcb7405c1d086 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -3495,7 +3495,7 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
// The Standard mandates shrink_to_fit() does not increase the capacity.
// With equal capacity keep the existing buffer. This avoids extra work
// due to swapping the elements.
- if (__allocation.count - 1 > capacity()) {
+ if (__allocation.count - 1 >= capacity()) {
__alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
return;
}
>From f393c2674d957db98800c477ce22bf71d4e5a192 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Sun, 16 Feb 2025 16:10:15 -0500
Subject: [PATCH 2/5] Add tests to ensure no buffer swapping with equal alloc
size
---
.../string.capacity/shrink_to_fit.pass.cpp | 60 +++++++++----
libcxx/test/support/increasing_allocator.h | 86 +++++++++++++++++++
2 files changed, 129 insertions(+), 17 deletions(-)
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index 2d901e7afe2b6..d8a2530a9a49e 100644
--- a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -61,28 +61,54 @@ TEST_CONSTEXPR_CXX20 bool test() {
test_string<std::basic_string<char, std::char_traits<char>, safe_allocator<char>>>();
#endif
- return true;
-}
-
#if TEST_STD_VER >= 23
-// https://github.com/llvm/llvm-project/issues/95161
-void test_increasing_allocator() {
- std::basic_string<char, std::char_traits<char>, increasing_allocator<char>> s{
- "String does not fit in the internal buffer"};
- std::size_t capacity = s.capacity();
- std::size_t size = s.size();
- s.shrink_to_fit();
- assert(s.capacity() <= capacity);
- assert(s.size() == size);
- LIBCPP_ASSERT(is_string_asan_correct(s));
+ { // Make sure shrink_to_fit never increases capacity
+ // See: https://github.com/llvm/llvm-project/issues/95161
+ std::basic_string<char, std::char_traits<char>, increasing_allocator<char>> s{
+ "String does not fit in the internal buffer"};
+ std::size_t capacity = s.capacity();
+ std::size_t size = s.size();
+ s.shrink_to_fit();
+ assert(s.capacity() <= capacity);
+ assert(s.size() == size);
+ LIBCPP_ASSERT(is_string_asan_correct(s));
+ }
+#endif
+
+ { // Ensure that the libc++ implementation of shrink_to_fit does NOT swap buffer with equal allocation sizes
+ { // Test with custom allocator with a minimum allocation size
+ std::basic_string<char, std::char_traits<char>, min_size_allocator<128, char> > s(
+ "A long string exceeding SSO limit but within min alloc size");
+ std::size_t capacity = s.capacity();
+ std::size_t size = s.size();
+ auto data = s.data();
+ s.shrink_to_fit();
+ assert(s.capacity() <= capacity);
+ assert(s.size() == size);
+ LIBCPP_ASSERT(is_string_asan_correct(s));
+ if (s.capacity() == capacity)
+ LIBCPP_ASSERT(s.data() == data);
+ }
+ { // Test with custom allocator with a minimum power of two allocation size
+ std::basic_string<char, std::char_traits<char>, pow2_allocator<char> > s(
+ "This is a long string that exceeds the SSO limit");
+ std::size_t capacity = s.capacity();
+ std::size_t size = s.size();
+ auto data = s.data();
+ s.shrink_to_fit();
+ assert(s.capacity() <= capacity);
+ assert(s.size() == size);
+ LIBCPP_ASSERT(is_string_asan_correct(s));
+ if (s.capacity() == capacity)
+ LIBCPP_ASSERT(s.data() == data);
+ }
+ }
+
+ return true;
}
-#endif // TEST_STD_VER >= 23
int main(int, char**) {
test();
-#if TEST_STD_VER >= 23
- test_increasing_allocator();
-#endif
#if TEST_STD_VER > 17
static_assert(test());
#endif
diff --git a/libcxx/test/support/increasing_allocator.h b/libcxx/test/support/increasing_allocator.h
index 30bd6f40c8dad..9eaa3e1ec5bff 100644
--- a/libcxx/test/support/increasing_allocator.h
+++ b/libcxx/test/support/increasing_allocator.h
@@ -10,6 +10,7 @@
#define TEST_SUPPORT_INCREASING_ALLOCATOR_H
#include <cstddef>
+#include <limits>
#include <memory>
#include "test_macros.h"
@@ -49,4 +50,89 @@ TEST_CONSTEXPR_CXX20 bool operator==(increasing_allocator<T>, increasing_allocat
return true;
}
+template <std::size_t MinAllocSize, typename T>
+class min_size_allocator {
+public:
+ using value_type = T;
+ min_size_allocator() = default;
+
+ template <typename U>
+ TEST_CONSTEXPR_CXX20 min_size_allocator(const min_size_allocator<MinAllocSize, U>&) TEST_NOEXCEPT {}
+
+#if TEST_STD_VER >= 23
+ TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
+ if (n < MinAllocSize)
+ n = MinAllocSize;
+ return std::allocator<T>{}.allocate_at_least(n);
+ }
+#endif // TEST_STD_VER >= 23
+
+ TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
+ if (n < MinAllocSize)
+ n = MinAllocSize;
+ return std::allocator<T>().allocate(n);
+ }
+
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+
+ template <typename U>
+ struct rebind {
+ using other = min_size_allocator<MinAllocSize, U>;
+ };
+};
+
+template <std::size_t MinAllocSize, typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool
+operator==(const min_size_allocator<MinAllocSize, T>&, const min_size_allocator<MinAllocSize, U>&) {
+ return true;
+}
+
+template <std::size_t MinAllocSize, typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool
+operator!=(const min_size_allocator<MinAllocSize, T>&, const min_size_allocator<MinAllocSize, U>&) {
+ return false;
+}
+
+template <typename T>
+class pow2_allocator {
+public:
+ using value_type = T;
+ pow2_allocator() = default;
+
+ template <typename U>
+ TEST_CONSTEXPR_CXX20 pow2_allocator(const pow2_allocator<U>&) TEST_NOEXCEPT {}
+
+#if TEST_STD_VER >= 23
+ TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
+ return std::allocator<T>{}.allocate_at_least(next_power_of_two(n));
+ }
+#endif // TEST_STD_VER >= 23
+
+ TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
+ return std::allocator<T>().allocate(next_power_of_two(n));
+ }
+
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+
+private:
+ TEST_CONSTEXPR_CXX20 std::size_t next_power_of_two(std::size_t n) const {
+ if ((n & (n - 1)) == 0)
+ return n;
+ for (std::size_t shift = 1; shift < std::numeric_limits<std::size_t>::digits; shift <<= 1) {
+ n |= n >> shift;
+ }
+ return n + 1;
+ }
+};
+
+template <typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool operator==(const pow2_allocator<T>&, const pow2_allocator<U>&) {
+ return true;
+}
+
+template <typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool operator!=(const pow2_allocator<T>&, const pow2_allocator<U>&) {
+ return false;
+}
+
#endif // TEST_SUPPORT_INCREASING_ALLOCATOR_H
>From af9b19fab142bd8f6e6a574c20422478e5388884 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Mon, 17 Feb 2025 11:08:18 -0500
Subject: [PATCH 3/5] Make libc++ tests explicit
---
.../basic.string/string.capacity/shrink_to_fit.pass.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index d8a2530a9a49e..d39610f57f747 100644
--- a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -86,8 +86,7 @@ TEST_CONSTEXPR_CXX20 bool test() {
assert(s.capacity() <= capacity);
assert(s.size() == size);
LIBCPP_ASSERT(is_string_asan_correct(s));
- if (s.capacity() == capacity)
- LIBCPP_ASSERT(s.data() == data);
+ LIBCPP_ASSERT(s.capacity() == capacity && s.data() == data);
}
{ // Test with custom allocator with a minimum power of two allocation size
std::basic_string<char, std::char_traits<char>, pow2_allocator<char> > s(
@@ -99,8 +98,7 @@ TEST_CONSTEXPR_CXX20 bool test() {
assert(s.capacity() <= capacity);
assert(s.size() == size);
LIBCPP_ASSERT(is_string_asan_correct(s));
- if (s.capacity() == capacity)
- LIBCPP_ASSERT(s.data() == data);
+ LIBCPP_ASSERT(s.capacity() == capacity && s.data() == data);
}
}
>From 96f01dd05091a4f1b520415680407c9e26f09b46 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Wed, 19 Feb 2025 10:55:20 -0500
Subject: [PATCH 4/5] Address philnik777's suggestions regarding libc++ test
organizations
---
.../string.capacity/shrink_to_fit.pass.cpp | 38 +++++++++++++++++--
.../string.capacity/shrink_to_fit.pass.cpp | 27 -------------
libcxx/test/support/increasing_allocator.h | 14 -------
3 files changed, 35 insertions(+), 44 deletions(-)
diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index 73b70d6f10bd5..3ac198655512b 100644
--- a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -12,12 +12,12 @@
// void shrink_to_fit(); // constexpr since C++20
-// Make sure we use an allocation returned by allocate_at_least if it is smaller than the current allocation
-// even if it contains more bytes than we requested
-
#include <cassert>
#include <string>
+#include "asan_testing.h"
+#include "increasing_allocator.h"
+
template <typename T>
struct oversizing_allocator {
using value_type = T;
@@ -37,6 +37,9 @@ bool operator==(oversizing_allocator<T>, oversizing_allocator<U>) {
return true;
}
+// Make sure we use an allocation returned by allocate_at_least if it is smaller than the current allocation
+// even if it contains more bytes than we requested.
+// Fix issue: https://github.com/llvm/llvm-project/pull/115659
void test_oversizing_allocator() {
std::basic_string<char, std::char_traits<char>, oversizing_allocator<char>> s{
"String does not fit in the internal buffer and is a bit longer"};
@@ -48,8 +51,37 @@ void test_oversizing_allocator() {
assert(s.size() == size);
}
+// Ensure that the libc++ implementation of shrink_to_fit does NOT swap buffer with equal allocation sizes
+void test_no_swap_with_equal_allocation_size() {
+ { // Test with custom allocator with a minimum allocation size
+ std::basic_string<char, std::char_traits<char>, min_size_allocator<128, char> > s(
+ "A long string exceeding SSO limit but within min alloc size");
+ std::size_t capacity = s.capacity();
+ std::size_t size = s.size();
+ auto data = s.data();
+ s.shrink_to_fit();
+ assert(s.capacity() <= capacity);
+ assert(s.size() == size);
+ assert(is_string_asan_correct(s));
+ assert(s.capacity() == capacity && s.data() == data);
+ }
+ { // Test with custom allocator with a minimum power of two allocation size
+ std::basic_string<char, std::char_traits<char>, pow2_allocator<char> > s(
+ "This is a long string that exceeds the SSO limit");
+ std::size_t capacity = s.capacity();
+ std::size_t size = s.size();
+ auto data = s.data();
+ s.shrink_to_fit();
+ assert(s.capacity() <= capacity);
+ assert(s.size() == size);
+ assert(is_string_asan_correct(s));
+ assert(s.capacity() == capacity && s.data() == data);
+ }
+}
+
int main(int, char**) {
test_oversizing_allocator();
+ test_no_swap_with_equal_allocation_size();
return 0;
}
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index d39610f57f747..9ca7825a4ba92 100644
--- a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -75,33 +75,6 @@ TEST_CONSTEXPR_CXX20 bool test() {
}
#endif
- { // Ensure that the libc++ implementation of shrink_to_fit does NOT swap buffer with equal allocation sizes
- { // Test with custom allocator with a minimum allocation size
- std::basic_string<char, std::char_traits<char>, min_size_allocator<128, char> > s(
- "A long string exceeding SSO limit but within min alloc size");
- std::size_t capacity = s.capacity();
- std::size_t size = s.size();
- auto data = s.data();
- s.shrink_to_fit();
- assert(s.capacity() <= capacity);
- assert(s.size() == size);
- LIBCPP_ASSERT(is_string_asan_correct(s));
- LIBCPP_ASSERT(s.capacity() == capacity && s.data() == data);
- }
- { // Test with custom allocator with a minimum power of two allocation size
- std::basic_string<char, std::char_traits<char>, pow2_allocator<char> > s(
- "This is a long string that exceeds the SSO limit");
- std::size_t capacity = s.capacity();
- std::size_t size = s.size();
- auto data = s.data();
- s.shrink_to_fit();
- assert(s.capacity() <= capacity);
- assert(s.size() == size);
- LIBCPP_ASSERT(is_string_asan_correct(s));
- LIBCPP_ASSERT(s.capacity() == capacity && s.data() == data);
- }
- }
-
return true;
}
diff --git a/libcxx/test/support/increasing_allocator.h b/libcxx/test/support/increasing_allocator.h
index 9eaa3e1ec5bff..bcddab08c9cce 100644
--- a/libcxx/test/support/increasing_allocator.h
+++ b/libcxx/test/support/increasing_allocator.h
@@ -59,14 +59,6 @@ class min_size_allocator {
template <typename U>
TEST_CONSTEXPR_CXX20 min_size_allocator(const min_size_allocator<MinAllocSize, U>&) TEST_NOEXCEPT {}
-#if TEST_STD_VER >= 23
- TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
- if (n < MinAllocSize)
- n = MinAllocSize;
- return std::allocator<T>{}.allocate_at_least(n);
- }
-#endif // TEST_STD_VER >= 23
-
TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
if (n < MinAllocSize)
n = MinAllocSize;
@@ -102,12 +94,6 @@ class pow2_allocator {
template <typename U>
TEST_CONSTEXPR_CXX20 pow2_allocator(const pow2_allocator<U>&) TEST_NOEXCEPT {}
-#if TEST_STD_VER >= 23
- TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
- return std::allocator<T>{}.allocate_at_least(next_power_of_two(n));
- }
-#endif // TEST_STD_VER >= 23
-
TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
return std::allocator<T>().allocate(next_power_of_two(n));
}
>From 98d8c3e4ccee6d91671c668ddac21dfcd6420a98 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Wed, 19 Feb 2025 17:16:10 -0500
Subject: [PATCH 5/5] Fix ASan new-delete-mismatch
---
.../basic.string/string.capacity/shrink_to_fit.pass.cpp | 2 +-
libcxx/test/support/increasing_allocator.h | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index 3ac198655512b..101838e8f525e 100644
--- a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -51,7 +51,7 @@ void test_oversizing_allocator() {
assert(s.size() == size);
}
-// Ensure that the libc++ implementation of shrink_to_fit does NOT swap buffer with equal allocation sizes
+// Make sure libc++ shrink_to_fit does NOT swap buffer with equal allocation sizes
void test_no_swap_with_equal_allocation_size() {
{ // Test with custom allocator with a minimum allocation size
std::basic_string<char, std::char_traits<char>, min_size_allocator<128, char> > s(
diff --git a/libcxx/test/support/increasing_allocator.h b/libcxx/test/support/increasing_allocator.h
index bcddab08c9cce..7e5aeaf7188e6 100644
--- a/libcxx/test/support/increasing_allocator.h
+++ b/libcxx/test/support/increasing_allocator.h
@@ -62,10 +62,10 @@ class min_size_allocator {
TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
if (n < MinAllocSize)
n = MinAllocSize;
- return std::allocator<T>().allocate(n);
+ return static_cast<T*>(::operator new(n * sizeof(T)));
}
- TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t) TEST_NOEXCEPT { ::operator delete(static_cast<void*>(p)); }
template <typename U>
struct rebind {
@@ -95,10 +95,10 @@ class pow2_allocator {
TEST_CONSTEXPR_CXX20 pow2_allocator(const pow2_allocator<U>&) TEST_NOEXCEPT {}
TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
- return std::allocator<T>().allocate(next_power_of_two(n));
+ return static_cast<T*>(::operator new(next_power_of_two(n) * sizeof(T)));
}
- TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t) TEST_NOEXCEPT { ::operator delete(static_cast<void*>(p)); }
private:
TEST_CONSTEXPR_CXX20 std::size_t next_power_of_two(std::size_t n) const {
More information about the libcxx-commits
mailing list