[libcxx] Align `recommend() + 1` by endian_factor #90292

vitalybuka · 2024-04-26T23:07:30Z

This is detected by asan after #83774

Allocation size will be divided by __endian_factor before storing. If it's not aligned,
we will not be able to recover allocation size to pass into __alloc_traits::deallocate.

we have code like this

 auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
    __p               = __allocation.ptr;
    __set_long_cap(__allocation.count);

void __set_long_cap(size_type __s) _NOEXCEPT {
    __r_.first().__l.__cap_     = __s / __endian_factor;
    __r_.first().__l.__is_long_ = true;
  }

size_type __get_long_cap() const _NOEXCEPT {
    return __r_.first().__l.__cap_ * __endian_factor;
  }

inline ~basic_string() {
    __annotate_delete();
    if (__is_long())
      __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
  }

__recommend() -> even size
std::__allocate_at_least(__alloc(), __recommend(__sz) + 1) - > not even size
__set_long_cap()- > lose one bit of size for __endian_factor == 2 (see / __endian_factor)
__alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap()) -> uses even size (see __get_long_cap)

llvmbot · 2024-04-26T23:08:02Z

@llvm/pr-subscribers-libcxx

Author: Vitaly Buka (vitalybuka)

Changes

[libcxx] Add assert into __set_long_cap
[libcxx] Align allocation to __set_long_cap and __get_long_cap match

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

1 Files Affected:

(modified) libcxx/include/string (+11-10)

diff --git a/libcxx/include/string b/libcxx/include/string
index 883bc1d7e5dc9f..ff9c7b81375c9c 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -868,7 +868,7 @@ private:
       __r_.first() = __rep();
       __set_short_size(__size);
     } else {
-      auto __capacity   = __recommend(__size) + 1;
+      auto __capacity   = __align_it<__endian_factor>(__recommend(__size) + 1);
       auto __allocation = __alloc_traits::allocate(__alloc(), __capacity);
       __begin_lifetime(__allocation, __capacity);
       __set_long_cap(__capacity);
@@ -1876,6 +1876,7 @@ private:
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __set_long_cap(size_type __s) _NOEXCEPT {
     __r_.first().__l.__cap_     = __s / __endian_factor;
     __r_.first().__l.__is_long_ = true;
+    _LIBCPP_ASSERT_INTERNAL(__s == __get_long_cap(), "Size must be __endian_factor aligned.");
   }
 
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type __get_long_cap() const _NOEXCEPT {
@@ -2201,7 +2202,7 @@ basic_string<_CharT, _Traits, _Allocator>::__init(const value_type* __s, size_ty
     __set_short_size(__sz);
     __p = __get_short_pointer();
   } else {
-    auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__reserve) + 1);
+    auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__recommend(__reserve) + 1));
     __p               = __allocation.ptr;
     __begin_lifetime(__p, __allocation.count);
     __set_long_pointer(__p);
@@ -2225,7 +2226,7 @@ basic_string<_CharT, _Traits, _Allocator>::__init(const value_type* __s, size_ty
     __set_short_size(__sz);
     __p = __get_short_pointer();
   } else {
-    auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
+    auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__recommend(__sz) + 1));
     __p               = __allocation.ptr;
     __begin_lifetime(__p, __allocation.count);
     __set_long_pointer(__p);
@@ -2250,7 +2251,7 @@ basic_string<_CharT, _Traits, _Allocator>::__init_copy_ctor_external(const value
   } else {
     if (__sz > max_size())
       __throw_length_error();
-    auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
+    auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__recommend(__sz) + 1));
     __p               = __allocation.ptr;
     __begin_lifetime(__p, __allocation.count);
     __set_long_pointer(__p);
@@ -2273,7 +2274,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
     __set_short_size(__n);
     __p = __get_short_pointer();
   } else {
-    auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__n) + 1);
+    auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__recommend(__n) + 1));
     __p               = __allocation.ptr;
     __begin_lifetime(__p, __allocation.count);
     __set_long_pointer(__p);
@@ -2338,7 +2339,7 @@ basic_string<_CharT, _Traits, _Allocator>::__init_with_size(_InputIterator __fir
     __p = __get_short_pointer();
 
   } else {
-    auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
+    auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__recommend(__sz) + 1));
     __p               = __allocation.ptr;
     __begin_lifetime(__p, __allocation.count);
     __set_long_pointer(__p);
@@ -2378,7 +2379,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
   size_type __cap =
       __old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms - 1;
   __annotate_delete();
-  auto __allocation = std::__allocate_at_least(__alloc(), __cap + 1);
+  auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__cap + 1));
   pointer __p       = __allocation.ptr;
   __begin_lifetime(__p, __allocation.count);
   if (__n_copy != 0)
@@ -2421,7 +2422,7 @@ void _LIBCPP_CONSTEXPR_SINCE_CXX20
   size_type __cap =
       __old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms - 1;
   __annotate_delete();
-  auto __allocation = std::__allocate_at_least(__alloc(), __cap + 1);
+  auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__cap + 1));
   pointer __p       = __allocation.ptr;
   __begin_lifetime(__p, __allocation.count);
   if (__n_copy != 0)
@@ -3254,14 +3255,14 @@ basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target
     __p        = __get_long_pointer();
   } else {
     if (__target_capacity > __cap) {
-      auto __allocation = std::__allocate_at_least(__alloc(), __target_capacity + 1);
+      auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__target_capacity + 1));
       __new_data        = __allocation.ptr;
       __target_capacity = __allocation.count - 1;
     } else {
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS
       try {
 #endif // _LIBCPP_HAS_NO_EXCEPTIONS
-        auto __allocation = std::__allocate_at_least(__alloc(), __target_capacity + 1);
+        auto __allocation = std::__allocate_at_least(__alloc(), __align_it<__endian_factor>(__target_capacity + 1));
         __new_data        = __allocation.ptr;
         __target_capacity = __allocation.count - 1;
 #ifndef _LIBCPP_HAS_NO_EXCEPTIONS

…#83774)" (#90299) https://lab.llvm.org/buildbot/#/builders/168/builds/20063 (should be fixed with #90292) More details in #83774 This reverts commit cf5a8b4.

philnik777

Could you add some more info here? I don't see what bug this fixes, and I don't understand why we can't drop a byte when setting the long cap.

jyknight · 2024-04-29T12:52:39Z

We pass the capacity directly to deallocate, e.g.:

  inline _LIBCPP_CONSTEXPR_SINCE_CXX20 ~basic_string() {
    __annotate_delete();
    if (__is_long())
      __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
  }

and for sized-delete, we are required to pass the actual size that was passed to allocate, not a rounded amount.

philnik777 · 2024-04-30T17:21:47Z

AFAICT we always return __align_it() - 1 from __recommend though? Adding 1 would bring us back to an aligned size. (Don't ask me why it's written this way)

vitalybuka · 2024-04-30T17:31:35Z

AFAICT we always return __align_it() - 1 from __recommend though? Adding 1 would bring us back to an aligned size. (Don't ask me why it's written this way)

The assert inserted there confirms that it's not true in some cases, on check-cxx, even without sanitizers.

philnik777 · 2024-04-30T17:33:20Z

AFAICT we always return __align_it() - 1 from __recommend though? Adding 1 would bring us back to an aligned size. (Don't ask me why it's written this way)

The assert inserted there confirms that it's not true in some cases, on check-cxx, even without sanitizers.

Ok, then what am I missing?

philnik777 · 2024-04-30T17:36:51Z

BTW it's expected that there might be a byte dropped if the allocator returns more than what we asked for. That's completely legal for us to do. (But I don't think we test that for string, so it shouldn't affect the tests)

vitalybuka · 2024-04-30T17:41:47Z

AFAICT we always return __align_it() - 1 from __recommend though? Adding 1 would bring us back to an aligned size. (Don't ask me why it's written this way)

The assert inserted there confirms that it's not true in some cases, on check-cxx, even without sanitizers.

Ok, then what am I missing?

Sorry, maybe we are discussing different things.
Are you questioning particular case? Some __align_it<> from this patch may be unnecessary.
but most stuff like this:

 auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1);
    __p               = __allocation.ptr;
    __begin_lifetime(__p, __allocation.count);
    __set_long_pointer(__p);
    __set_long_cap(__allocation.count);
    __set_long_size(__sz);

__recommend() -> even size
std::__allocate_at_least(__alloc(), __recommend(__sz) + 1) - > not even size
__set_long_cap() - > lose one bit of size for __endian_factor == 2
__alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap()); -> uses even size

philnik777 · 2024-04-30T17:46:36Z

What I'm saying is that __recommend() should always return an odd number (__recommend() should return the same value as capacity() IIRC), and adding one (for the null terminator) would result in an even number.

vitalybuka · 2024-04-30T17:47:19Z

BTW it's expected that there might be a byte dropped if the allocator returns more than what we asked for. That's completely legal for us to do. (But I don't think we test that for string, so it shouldn't affect the tests)

Hm, I would expect allocator should still accept the same size for new/delete, even if it returns more. Otherwise using those operators will be very annoying.

philnik777 · 2024-04-30T17:50:17Z

BTW it's expected that there might be a byte dropped if the allocator returns more than what we asked for. That's completely legal for us to do. (But I don't think we test that for string, so it shouldn't affect the tests)

Hm, I would expect allocator should still accept the same size for new/delete, even if it returns more. Otherwise using those operators will be very annoying.

Yes, it does. With allocate_at_least you can give it anything between the requested size and the actually allocated size. Because of that it's fine for us to drop a byte that's been promised by the allocator.

vitalybuka · 2024-04-30T17:55:27Z

Yes, it does. With allocate_at_least you can give it anything between the requested size and the actually allocated size. Because of that it's fine for us to drop a byte that's been promised by the allocator.

So, I am not sure then what are you asking?
new/delete size should match, without the patch it does not match.
I assume we can switch not non-sized delete, but it's less efficient.

philnik777 · 2024-04-30T17:59:17Z

I'm asking where we get the mismatch. My current mental model is that we always allocate an even number of elements (due to __recommend() returning an odd number plus one resulting in an even number), which apparently isn't the case. In which part of the code is my mental model failing? Your current solution looks like a band-aid to me and not like it's fixing the underlying issue.

vitalybuka · 2024-04-30T18:07:13Z

I'm asking where we get the mismatch. My current mental model is that we always allocate an even number of elements (due to __recommend() returning an odd number plus one resulting in an even number), which apparently isn't the case. In which part of the code is my mental model failing?

Got it, will reproduce with details soon.

Your current solution looks like a band-aid to me and not like it's fixing the underlying issue.

Sorry, it's just how I understood the code, will be happy to improve.

vitalybuka · 2024-04-30T18:13:01Z

libcxx/include/string

@@ -1876,6 +1876,7 @@ private:
  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __set_long_cap(size_type __s) _NOEXCEPT {
    __r_.first().__l.__cap_     = __s / __endian_factor;
    __r_.first().__l.__is_long_ = true;
+    _LIBCPP_ASSERT_INTERNAL(__s == __get_long_cap(), "Size must be __endian_factor aligned.");


@philnik777
Note, this assert is enough to reproduce the issue on LIT_FILTER=string ninja check-cxx, in case it would be easy for you to debug yourself.

Or maybe not...
I'll be back with details soon.

I see:

https://github.com/llvm/llvm-project/actions/runs/8899718744/job/24441331959?pr=90645

Also my local run if we apply only #90645

gdb runtimes/runtimes-bins/test/std/strings/basic.string/string.modifiers/string_replace/Output/size_size_pointer_size.pass.cpp.dir/t.tmp.exe

#0 __pthread_kill_implementation (threadid=<optimized out>, signo=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44 #1 0x00007ffff7b691cf in __pthread_kill_internal (signo=<optimized out>, threadid=<optimized out>) at ./nptl/pthread_kill.c:89 #2 __GI___pthread_kill (threadid=<optimized out>, signo=<optimized out>) at ./nptl/pthread_kill.c:89 #3 0x00007ffff7cb3e90 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #4 0x00007ffff7b054b2 in __GI_abort () at ./stdlib/abort.c:79 #5 0x00007ffff7f44bce in std::__1::__libcpp_verbose_abort (format=0x7ffff7e9c1f8 "%s") at /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/libcxx/src/verbose_abort.cpp:74 #6 0x00007ffff7f2da14 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__set_long_cap[abi:de190000](unsigned long) (this=0x7fffffffc7b0, __s=25) at include/c++/v1/string:1879 #7 0x00007ffff7f3ab96 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::__init (this=0x7fffffffc7b0, __s=0x5555555e638d "1234567890123456789bcde", __sz=23) at include/c++/v1/string:2233 #8 0x0000555555584334 in std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >::basic_string[abi:de190000]<0> () #9 0x0000555555557441 in bool test0<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >() () #10 0x00005555555552d9 in void test<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >() () #11 0x00005555555552bb in main ()

__init(const value_type* __s, size_type __sz = 23)

__recommend(23) returns 24, because 23 is __min_cap

then as in the description we allocate 25, but store 25 / 2

and next time we delete 24

philnik777 · 2024-04-30T18:55:51Z

I've looked into this a bit more and it turns out that we have two unrelated bugs in __recommend. First, we add one if __guess == __min_cap, and second our minimum alignment we give to __align_it is one, instead of the __endian_factor.

I think

diff --git a/libcxx/include/string b/libcxx/include/string
index 883bc1d7e5dc..13af92fbce3c 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1959,10 +1959,10 @@ private:
     if (__s < __min_cap) {
       return static_cast<size_type>(__min_cap) - 1;
     }
-    const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : 1;
+    const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : __endian_factor;
     size_type __guess          = __align_it<__boundary>(__s + 1) - 1;
     if (__guess == __min_cap)
-      ++__guess;
+      __guess += 2;
     return __guess;
   }

should fix the issues you're seeing.

vitalybuka · 2024-04-30T20:34:16Z

I've looked into this a bit more and it turns out that we have two unrelated bugs in __recommend. First, we add one if __guess == __min_cap, and second our minimum alignment we give to __align_it is one, instead of the __endian_factor.

I think

diff --git a/libcxx/include/string b/libcxx/include/string
index 883bc1d7e5dc..13af92fbce3c 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1959,10 +1959,10 @@ private:
     if (__s < __min_cap) {
       return static_cast<size_type>(__min_cap) - 1;
     }
-    const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : 1;
+    const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : __endian_factor;
     size_type __guess          = __align_it<__boundary>(__s + 1) - 1;
     if (__guess == __min_cap)
-      ++__guess;
+      __guess += 2;
     return __guess;
   }

should fix the issues you're seeing.

My last reply written before reading this, but looks like they are consistent.

vitalybuka · 2024-04-30T21:39:27Z

libcxx/include/string

    size_type __guess          = __align_it<__boundary>(__s + 1) - 1;
    if (__guess == __min_cap)
-      ++__guess;
+      __guess += 2;


works with ASSERT, and likely will work with asan
Should __guess += 2 be replaced with __guess += __endian_factor if we want to keep __endian_factor alignment?

works with ASSERT, and likely will work with asan Should __guess += 2 be replaced with __guess += __endian_factor if we want to keep __endian_factor alignment?

Assuming __endian_factor unlikely will be >2 in future, 2 will work, but __endian_factor is better for reader?

Yeah, I think __endian_factor would be the right call. Maybe we should rename it into something like __min_capacity_alignment, since that's what it basically is? Just a though, not a request for this patch.

philnik777

To unblock landing #83774 I'm happy with this patch if you remove the _LIBCPP_ASSERT_INTERNAL (since it may not actually be correct) and change __guess += 2 to __guess += __endian_factor.

tstellar · 2024-06-12T01:03:56Z

Basically, folks passing the -fsized-deallocation Clang flag (which is off by default in LLVM 18) will get the wrong deallocation size passed to their operator delete when storage has been allocated/deallocated by std::string. I don't know what the result of passing the wrong size will usually be, I guess it depends on what the custom operator delete does, but I could definitely imagine it being a correctness concern.

Is there a reason why gcc has this on by default, but clang doesn't?

ckennelly · 2024-06-12T01:05:45Z

I'm unable to say whether or not this is a problem for Google's codebase, since @alk speculated upthread that it might be string ABI dependent. I'll see if I can loop some folks in who would know.

Either way, sized deallocation is an important performance optimization in modern memory allocators to avoid a costly pointer-to-size lookup (in TCMalloc's case, a 2/3-level radix tree walk). Passing the wrong size is UB and diagnosed by ASan as such, and in practice means we might place the deallocated object on the wrong freelist.

ldionne · 2024-06-12T01:08:21Z

Is there a reason why gcc has this on by default, but clang doesn't?

Basically, we had trouble with rolling it out. It's a wide-reaching change to deploy in the wild because it's effectively an ABI change (call sites that used to call operator delete(void*) will now sometimes call operator delete(void*, size_t)), and it just took a long time for us to be comfortable and able to make it the default. For example, enabling -fsized-deallocation by default required many rounds of fixing tests and other stuff just within LLVM. Or at least that's my understanding.

tstellar · 2024-06-12T01:13:56Z

OK, if we backport this fix, is there someone willing to do some extra testing on the release branch? I would like to get some kind of confirmation that the backport fixes the issues and that it doesn't introduce another regression?

ldionne · 2024-06-12T01:22:00Z

@vitalybuka found this with sanitizers (and -fsized-deallocation) originally, perhaps he can help with running the tests in that same configuration on the release branch with the cherry-pick?

vitalybuka · 2024-06-12T01:42:54Z

@vitalybuka found this with sanitizers (and -fsized-deallocation) originally, perhaps he can help with running the tests in that same configuration on the release branch with the cherry-pick?

Sure, send me PR, or a branch with a fix.

jyknight · 2024-06-12T02:05:44Z

There seems to be a claim that Google's using this flag at large scale internally. If it's possible to detail the impact of this regression a bit more, it might help to give at least one "real world" point of view on this.

Google has indeed been using -fsized-deallocation with tcmalloc (which uses the deallocation size information) internally for a long time. However, we also use the libc++ unstable abi, which enables the "alternate" string layout. In that version, the "endian_factor" constant is 1 on little endian machines, rather than 2, so this bug doesn't appear.

ldionne · 2024-06-12T15:22:52Z

/cherry-pick d129ea8

llvmbot · 2024-06-12T15:28:03Z

Failed to cherry-pick: d129ea8

https://github.com/llvm/llvm-project/actions/runs/9485332934

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

DimitryAndric · 2024-06-12T15:34:47Z

I also had to cherry-pick @ldionne's 382f70a which is a formatting-only change, otherwise d129ea8 won't apply cleanly.

DimitryAndric · 2024-06-12T15:37:46Z

/cherry-pick 382f70a d129ea8

This is detected by asan after llvm#83774 Allocation size will be divided by `__endian_factor` before storing. If it's not aligned, we will not be able to recover allocation size to pass into `__alloc_traits::deallocate`. we have code like this ``` auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1); __p = __allocation.ptr; __set_long_cap(__allocation.count); void __set_long_cap(size_type __s) _NOEXCEPT { __r_.first().__l.__cap_ = __s / __endian_factor; __r_.first().__l.__is_long_ = true; } size_type __get_long_cap() const _NOEXCEPT { return __r_.first().__l.__cap_ * __endian_factor; } inline ~basic_string() { __annotate_delete(); if (__is_long()) __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap()); } ``` 1. __recommend() -> even size 2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not even size 3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2 (see `/ __endian_factor`) 4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap())` -> uses even size (see `__get_long_cap`) (cherry picked from commit d129ea8)

llvmbot · 2024-06-12T15:43:20Z

/pull-request #95264

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279560#c10 and llvm/llvm-project#90292

This is detected by asan after #83774 Allocation size will be divided by `__endian_factor` before storing. If it's not aligned, we will not be able to recover allocation size to pass into `__alloc_traits::deallocate`. we have code like this ``` auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1); __p = __allocation.ptr; __set_long_cap(__allocation.count); void __set_long_cap(size_type __s) _NOEXCEPT { __r_.first().__l.__cap_ = __s / __endian_factor; __r_.first().__l.__is_long_ = true; } size_type __get_long_cap() const _NOEXCEPT { return __r_.first().__l.__cap_ * __endian_factor; } inline ~basic_string() { __annotate_delete(); if (__is_long()) __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap()); } ``` 1. __recommend() -> even size 2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not even size 3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2 (see `/ __endian_factor`) 4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap())` -> uses even size (see `__get_long_cap`) (cherry picked from commit d129ea8)

h-vetinari · 2024-06-13T04:06:51Z

Is there a reason why gcc has this on by default, but clang doesn't?

Clang 19 finally enabled it by default (as of 130e93c). IMO this makes it even more important to be able to provide a smooth upgrade path (i.e. people being able to test -fsized-deallocation separately before bumping to clang 19), aside from the inherent importance of being a C++14 feature (i.e. very old, but which turned out to be exceptionally hard to roll out).

This is detected by asan after llvm#83774 Allocation size will be divided by `__endian_factor` before storing. If it's not aligned, we will not be able to recover allocation size to pass into `__alloc_traits::deallocate`. we have code like this ``` auto __allocation = std::__allocate_at_least(__alloc(), __recommend(__sz) + 1); __p = __allocation.ptr; __set_long_cap(__allocation.count); void __set_long_cap(size_type __s) _NOEXCEPT { __r_.first().__l.__cap_ = __s / __endian_factor; __r_.first().__l.__is_long_ = true; } size_type __get_long_cap() const _NOEXCEPT { return __r_.first().__l.__cap_ * __endian_factor; } inline ~basic_string() { __annotate_delete(); if (__is_long()) __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap()); } ``` 1. __recommend() -> even size 2. `std::__allocate_at_least(__alloc(), __recommend(__sz) + 1)` - > not even size 3. ` __set_long_cap() `- > lose one bit of size for __endian_factor == 2 (see `/ __endian_factor`) 4. `__alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap())` -> uses even size (see `__get_long_cap`) (cherry picked from commit d129ea8)

This is regression test for #90292. Allocator used in test is very similar to test_allocator. However, reproducer requires size_type of the string to be 64bit, but test_allocator uses 32bit. 32bit size_type makes `sizeof(string::__long)` to be 16, but the alignment issue fixed with #90292 is only triggered with default `sizeof(string::__long)` which is 24. Fixes #92128. --------- Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

…10210) This is regression test for llvm#90292. Allocator used in test is very similar to test_allocator. However, reproducer requires size_type of the string to be 64bit, but test_allocator uses 32bit. 32bit size_type makes `sizeof(string::__long)` to be 16, but the alignment issue fixed with llvm#90292 is only triggered with default `sizeof(string::__long)` which is 24. Fixes llvm#92128. --------- Co-authored-by: Louis Dionne <ldionne.2@gmail.com>

vitalybuka requested a review from a team as a code owner April 26, 2024 23:07

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 26, 2024

vitalybuka requested review from ldionne and EricWF April 26, 2024 23:08

vitalybuka changed the title ~~fixnew~~ [libcxx] Align allocation to __set_long_cap and __get_long_cap match Apr 26, 2024

vitalybuka changed the title ~~[libcxx] Align allocation to __set_long_cap and __get_long_cap match~~ [libcxx] Align allocation to match __set_long_cap and __get_long_cap Apr 26, 2024

vitalybuka mentioned this pull request Apr 26, 2024

Revert "[clang] Enable sized deallocation by default in C++14 onwards (#83774)" #90299

Merged

philnik777 requested changes Apr 27, 2024

View reviewed changes

wangpc-pp mentioned this pull request Apr 29, 2024

Reland "[clang] Enable sized deallocation by default in C++14 onwards" #90373

Merged

ldionne assigned philnik777 Apr 30, 2024

vitalybuka requested a review from philnik777 April 30, 2024 17:56

vitalybuka commented Apr 30, 2024

View reviewed changes

philnik777 reviewed May 1, 2024

View reviewed changes

ldionne added this to the LLVM 18.X Release milestone Jun 12, 2024

llvmbot added the release:cherry-pick-failed label Jun 12, 2024

llvmbot removed the release:cherry-pick-failed label Jun 12, 2024

ldionne mentioned this pull request Jun 12, 2024

release/18.x: [libcxx] Align __recommend() + 1 by __endian_factor (#90292) #95264

Merged

alk added a commit to gperftools/gperftools that referenced this pull request Jun 12, 2024

allow disabling actual versus given sized delete checking

5284f38

See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279560#c10 and llvm/llvm-project#90292

EricWF removed their request for review June 13, 2024 14:44

Dramaga11 approved these changes Jul 1, 2024

View reviewed changes

vitalybuka mentioned this pull request Sep 27, 2024

[libc++][string] Add regression test for sized new/delete bug #110210

Merged

[libcxx] Align __recommend() + 1 by __endian_factor #90292

[libcxx] Align __recommend() + 1 by __endian_factor #90292

Conversation

vitalybuka commented Apr 26, 2024 • edited Loading

llvmbot commented Apr 26, 2024

philnik777 left a comment

Choose a reason for hiding this comment

jyknight commented Apr 29, 2024

philnik777 commented Apr 30, 2024

vitalybuka commented Apr 30, 2024 • edited Loading

philnik777 commented Apr 30, 2024

philnik777 commented Apr 30, 2024

vitalybuka commented Apr 30, 2024

philnik777 commented Apr 30, 2024 • edited Loading

vitalybuka commented Apr 30, 2024

philnik777 commented Apr 30, 2024

vitalybuka commented Apr 30, 2024

philnik777 commented Apr 30, 2024

vitalybuka commented Apr 30, 2024

vitalybuka Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

vitalybuka Apr 30, 2024

Choose a reason for hiding this comment

vitalybuka Apr 30, 2024

Choose a reason for hiding this comment

philnik777 commented Apr 30, 2024

vitalybuka commented Apr 30, 2024

vitalybuka Apr 30, 2024

Choose a reason for hiding this comment

vitalybuka Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

philnik777 May 1, 2024

Choose a reason for hiding this comment

vitalybuka May 1, 2024

Choose a reason for hiding this comment

philnik777 left a comment

Choose a reason for hiding this comment

tstellar commented Jun 12, 2024

ckennelly commented Jun 12, 2024

ldionne commented Jun 12, 2024 • edited Loading

tstellar commented Jun 12, 2024

ldionne commented Jun 12, 2024

vitalybuka commented Jun 12, 2024

jyknight commented Jun 12, 2024

ldionne commented Jun 12, 2024

llvmbot commented Jun 12, 2024

DimitryAndric commented Jun 12, 2024

DimitryAndric commented Jun 12, 2024

llvmbot commented Jun 12, 2024

h-vetinari commented Jun 13, 2024 • edited Loading

[libcxx] Align `recommend() + 1` by endian_factor #90292

[libcxx] Align `recommend() + 1` by endian_factor #90292

vitalybuka commented Apr 26, 2024 •

edited

Loading

vitalybuka commented Apr 30, 2024 •

edited

Loading

philnik777 commented Apr 30, 2024 •

edited

Loading

vitalybuka Apr 30, 2024 •

edited

Loading

vitalybuka Apr 30, 2024 •

edited

Loading

ldionne commented Jun 12, 2024 •

edited

Loading

h-vetinari commented Jun 13, 2024 •

edited

Loading