[libc++][test] Refactor increasing_allocator

[winner245]

The increasing_allocator<T> class, originally introduced to test shrink_to_fit for vector and string (#95161), does not satisfy Cpp17Allocator requirements because its allocate(n) member function may allocate more memory than requested. However, the standard ([allocator.requirements]/36) mandates that a.allocate(n) must allocate memory for an array of exactly n objects of type T.

a.allocate(n)
35 Result: XX​::​pointer
36 Effects: Memory is allocated for an array of n T and such an object is created but array elements are not constructed.
[Example 1: When reusing storage denoted by some pointer value p, launder(reinterpret_cast<T*>(new (p) byte[n * sizeof(T)])) can be used to implicitly create a suitable array object and obtain a pointer to it. — end example]

This PR addresses the issue by modifying increasing_allocator<T>::allocate(n) such that it strictly allocates for exactly n objects. Note that this change does not affect the existing tests for shrink_to_fit, as those tests only utilize the allocate_at_least member function, which is already standard conforming.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The increasing_allocator<T> class, originally introduced to test shrink_to_fit for vector and string (#95161), does not satisfy Cpp17Allocator requirements because its allocate(n) member function may allocate more memory than requested. However, the standard ([allocator.requirements]/36) mandates that a.allocate(n) must allocate memory for an array of exactly n objects of type T.

> a.allocate(n)
35 Result: XX​::​pointer
36 Effects: Memory is allocated for an array of n T and such an object is created but array elements are not constructed.
[Example 1: When reusing storage denoted by some pointer value p, launder(reinterpret_cast<T*>(new (p) byte[n * sizeof(T)])) can be used to implicitly create a suitable array object and obtain a pointer to it. — end example]

This PR addresses the issue by modifying increasing_allocator<T>::allocate(n) such that it strictly allocates for exatcly n objects. Note that this change does not affect the existing tests for shrink_to_fit, as those tests only utilize the allocate_at_least member function, which is already standard conforming.

---

Full diff: https://github.com/llvm/llvm-project/pull/115671.diff

3 Files Affected:

• (modified) libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp (+1-1)
• (modified) libcxx/test/std/containers/sequences/vector/vector.capacity/shrink_to_fit.pass.cpp (+1-1)
• (modified) libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp (+2-2)

diff --git a/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
index f8bcee31964bbb..136b151efa29ef 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
@@ -55,7 +55,7 @@ struct increasing_allocator {
     min_elements += 1000;
     return std::allocator<T>{}.allocate_at_least(n);
   }
-  constexpr T* allocate(std::size_t n) { return allocate_at_least(n).ptr; }
+  constexpr T* allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
   constexpr void deallocate(T* p, std::size_t n) noexcept { std::allocator<T>{}.deallocate(p, n); }
 };
 
diff --git a/libcxx/test/std/containers/sequences/vector/vector.capacity/shrink_to_fit.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.capacity/shrink_to_fit.pass.cpp
index e39afb2d48f0a0..97d67dac2baa8c 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.capacity/shrink_to_fit.pass.cpp
@@ -87,7 +87,7 @@ struct increasing_allocator {
     min_elements += 1000;
     return std::allocator<T>{}.allocate_at_least(n);
   }
-  constexpr T* allocate(std::size_t n) { return allocate_at_least(n).ptr; }
+  constexpr T* allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
   constexpr void deallocate(T* p, std::size_t n) noexcept { std::allocator<T>{}.deallocate(p, n); }
 };
 
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 6f5e43d1341f53..68360329308bab 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
@@ -79,8 +79,8 @@ struct increasing_allocator {
     min_bytes += 1000;
     return {static_cast<T*>(::operator new(allocation_amount)), allocation_amount / sizeof(T)};
   }
-  T* allocate(std::size_t n) { return allocate_at_least(n).ptr; }
-  void deallocate(T* p, std::size_t) noexcept { ::operator delete(static_cast<void*>(p)); }
+  T* allocate(std::size_t n) { return std::allocator<T>{}.allocate(n); }
+  void deallocate(T* p, std::size_t) noexcept { std::allocator<T>{}.deallocate(p, n); }
 };
 
 template <typename T, typename U>

[ldionne]

Can we factor out the code into a single location?

[philnik777]

I feel like the reasoning is backwards here. AFAICT the only problem is that we're mismatching the n we pass to allocator::deallocate with the n we're passing to allocator::allocate. Nothing requires that there is space for exactly n elements in an allocation - there has to be space for at least that. In fact, most allocators return more than the requested amount in at least some cases, which is the whole reason allocate_at_least was introduced.

[ldionne]

@winner245 Actually, how did you come across this issue? Did our test suite fail with another Standard library somehow?

[winner245]

Because I just realized that the same issue #95161 applies to __split_buffer::shrink_to_fit, which can increase the capacity of the underlying buffer when the allocator's allocate_at_least allocates more memory than requested.

I was trying to find a different way to solve the shrink_to_fit issue because I think the root cause to all these shink_to_fit issues is that we used allocate_at_least instead of allocate. If we just revert back to allocate, I believe all the shinrk_to_fit issue would automatically be gone. This is because the standard ([allocator.requirements]/36) mandates that a.allocate(n) must allocate memory for exactly n objects of type T.

[winner245]

I personally do not like the current solution, because allocate_at_least may end up allocating more memory than requested. Then we would have to immediately discard the newly allocated memory. If we revert back to allocate, we wouldn't have the problem.

[philnik777]

I personally do not like the current solution, because allocate_at_least may end up allocating more memory than requested. Then we would have to immediately discard the newly allocated memory. If we revert back to allocate, we wouldn't have the problem.

The intention of allocate_at_least is that the allocator tells us how much it overallocated due to its allocation algorithm, not that it actually allocates more. If you implement the interface properly allocate will only hide the overallocation, not prevent it.

[frederick-vs-ja]

@winner245 Actually, how did you come across this issue? Did our test suite fail with another Standard library somehow?

Actually, the tests failed with MSVC STL (or MSVC, the compiler). MSVC detects size mismatch of deallocation in constant evaluation. I guess we can't reliably perform over allocation in constant evaluation.

(It seems that VCRuntime can detect size mismatch at run time.)

[ldionne]

The intention of allocate_at_least is that the allocator tells us how much it overallocated due to its allocation algorithm, not that it actually allocates more. If you implement the interface properly allocate will only hide the overallocation, not prevent it.

I agree, and I don't see how this makes the allocator more conforming than it used to be. However, I do think the code makes more sense after the patch than it did before the patch, because implementing allocate() using std::allocator::allocate makes more sense than doing so with allocate_at_least.

Actually, the tests failed with MSVC STL (or MSVC, the compiler). MSVC detects size mismatch of deallocation in constant evaluation. I guess we can't reliably perform over allocation in constant evaluation.

constexpr std::allocation_result<T*> allocate_at_least(std::size_t n);
constexpr T* allocate(std::size_t n) {
  auto res = allocate_at_least(n);
  return res.ptr;
}
constexpr void deallocate(T* p, std::size_t n) noexcept {
  // n may be wrong: it must match res.count which may not be the case if we used allocate_at_least above
  std::allocator<T>{}.deallocate(p, n);
}

Is that what you're saying? I would understand why the current code is a problem in that case, however http://eel.is/c++draft/allocator.members#10.1 says:

If p is memory that was obtained by a call to allocate_at_least, let ret be the value returned and req be the value passed as the first argument to that call. p is equal to ret.ptr and n is a value such that req  ≤ n  ≤ ret.count.

In other words, I think it's supposed to be just fine to call std::allocator::deallocate with a n that is less than the count returned by a previous allocate_at_least.

[philnik777]

The intention of allocate_at_least is that the allocator tells us how much it overallocated due to its allocation algorithm, not that it actually allocates more. If you implement the interface properly allocate will only hide the overallocation, not prevent it.

I agree, and I don't see how this makes the allocator more conforming than it used to be. However, I do think the code makes more sense after the patch than it did before the patch, because implementing allocate() using std::allocator::allocate makes more sense than doing so with allocate_at_least.

I'm not disagreeing here. The code is an improvement, but I don't think the commit message makes sense.

Actually, the tests failed with MSVC STL (or MSVC, the compiler). MSVC detects size mismatch of deallocation in constant evaluation. I guess we can't reliably perform over allocation in constant evaluation.

constexpr std::allocation_result<T*> allocate_at_least(std::size_t n);
constexpr T* allocate(std::size_t n) {
  auto res = allocate_at_least(n);
  return res.ptr;
}
constexpr void deallocate(T* p, std::size_t n) noexcept {
  // n may be wrong: it must match res.count which may not be the case if we used allocate_at_least above
  std::allocator<T>{}.deallocate(p, n);
}

Is that what you're saying? I would understand why the current code is a problem in that case, however http://eel.is/c++draft/allocator.members#10.1 says:

If p is memory that was obtained by a call to allocate_at_least, let ret be the value returned and req be the value passed as the first argument to that call. p is equal to ret.ptr and n is a value such that req  ≤ n  ≤ ret.count.

In other words, I think it's supposed to be just fine to call std::allocator::deallocate with a n that is less than the count returned by a previous allocate_at_least.

The calling code is fine. The mismatch between the std::allocator::allocate() and std::allocator::deallocate() is the problem. Note how increasing_allocator::allocate_at_least calls std::allocator::allocate(n + 1000) in some cases, but increasing_allocator::deallocate() always just calls std::allocator::deallocate(n).

Actually, I just noticed that the current code is still wrong, since there is still potentially the same mismatch when calling increasing_allocator::allocate_at_least and increasing_allocator::deallocate.

[winner245]

Thank you all for the very informative discussions here. I agree that the current solution for shrink_to_fit is the best compromise we have at this point. In response to @philnik777's comment that "allocate will only hide the overallocation, not prevent it," I think this PR now serves as a refactoring:

• Currently, the increasing_allocator<T> class appears in three different files. We should factor it out into a single location.
• Semantically, it makes more sense to let allocate() call std::allocator::allocate, not allocate_at_least (and let allocate_at_least() call std::allocator::allocate_at_least).