[libc++] Fix no-op shrink_to_fit for vector<bool>

[winner245]

This PR addresses an issue where the shrink_to_fit function in vector<bool> is effectively a no-op, meaning it will never shrink the capacity.

Fixes #122502.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The current implementation of vector<bool>::shrink_to_fit contains a logical error that prevents the function from doing anything practical, i.e., it will never shrink the capacity unless an arithmetic underflow happens. This is because the implementation checks the following condition

if (__external_cap_to_internal(size()) > __cap_) {
  try {
    do_shrink();
  } catch (...) {
  }  
}

However, this condition is always false (with one exception), as the number of used internal words __external_cap_to_internal(size()) will never exceed the internal storage capacity (__cap_). The only exception for this condition to be true is when size() == 0, where the evaluation of __external_cap_to_internal(0) underflows and wraps around to become size_type(-1). But it is highly unlikely that this behavior was intended, because it relies on an arithmetic underflow. Even if it were intentional, a more straightforward approach would have been:

if (empty())
  deallocate();

which avoids the try/catch clause and becomes much cleaner and simpler.

While the current implementation technically conforms to the standard—since the shrink-to-fit request is non-binding—it appears to be a logical error to me, particularly since a similar logical error was found in the reserve() function of __split_buffer in #105681, which remained uncorrected until last month (#115735).

This PR addresses the issue by modifying shrink_to_fit to actually perform the necessary capacity reduction, ensuring it behaves as expected.

---

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

1 Files Affected:

• (modified) libcxx/include/__vector/vector_bool.h (+4-2)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..190ea2585b7821 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -841,11 +841,13 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::reserve(size_type _
 
 template <class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::shrink_to_fit() _NOEXCEPT {
-  if (__external_cap_to_internal(size()) > __cap_) {
+  if (__external_cap_to_internal(size()) < __cap_) {
 #if _LIBCPP_HAS_EXCEPTIONS
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
-      vector(*this, allocator_type(__alloc_)).swap(*this);
+      vector __v(*this, allocator_type(__alloc_));
+      if (__v.__cap_ < __cap_)
+        __v.swap(*this);
 #if _LIBCPP_HAS_EXCEPTIONS
     } catch (...) {
     }

[asmok-g]

Thanks for the fix. This fixes some google code broken by #120577.

[winner245]

@asmok-g Thanks for your feedback! The previous change #120577 was an internal fix. I'm curious about how it caused issues with Google code, and how my current PR fixes that. Could you provide more details or context about the specific problems it created? This will help me understand the impact and ensure a comprehensive fix.

[asmok-g]

We got HeapLeakChecker: memory leak reports. Running under asan for more details, the stack trace shows the leak is related to the allocation of a vector_bool. I unfortunately can only provide a repro next week, as I'm out today. But can we still carry on with this fix as is for the time being ?

[alexfh]

@asmok-g Thanks for your feedback! The previous change #120577 was an internal fix. I'm curious about how it caused issues with Google code, and how my current PR fixes that. Could you provide more details or context about the specific problems it created? This will help me understand the impact and ensure a comprehensive fix.

I suspect that the issues we've seen may be caused by the same effect as described in #122502. If we find a distinct problem, we'll let you know.

[ldionne]

LGTM with a few comments, thanks a lot for fixing this!