[libc++] Fix basic_string::shrink_to_fit for constant evaluation

[frederick-vs-ja]

Currently, when the string shrink into the SSO buffer, the __rep_.__s member isn't activated before the traits_type::copy call
yet, so internal __builtin_memmove call writing to the buffer causes constant evaluation failure. The existing test coverage seems a bit defective and doesn't cover this case - shrink_to_fit is called on the copy of string after erasure, not the original string object.

This PR reorders the __set_short_size call, which starts the lifetime of the SSO buffer, before the copy operation. Test coverage is achieved by calling shrink_to_fit on the original erased string.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

Currently, when the string shrink into the SSO buffer, the lifetime of the buffer hasn't begun before copying characters into it, so the subsequent copy operation raises UB and thus causes constant evaluation failure.

The existing test coverage seems a bit defective - shrink_to_fit is called on the copy of string after erasure, not the original string object.

This PR reorders the __set_short_size call, which starts the lifetime of the SSO buffer, before the copy operation. Test coverage is achieved by calling shrink_to_fit on the original erased string.

---

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

2 Files Affected:

• (modified) libcxx/include/string (+1-1)
• (modified) libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp (+1-1)

diff --git a/libcxx/include/string b/libcxx/include/string
index 4f05e211919f3..67c4c537eb3b5 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -3434,8 +3434,8 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
 
   if (__fits_in_sso(__target_capacity)) {
     __annotation_guard __g(*this);
-    traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);
     __set_short_size(__size);
+    traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);
     __alloc_traits::deallocate(__alloc_, __ptr, __cap);
     return;
   }
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 9ca7825a4ba92..a3b16c8da16cb 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
@@ -19,7 +19,7 @@
 #include "test_macros.h"
 
 template <class S>
-TEST_CONSTEXPR_CXX20 void test(S s) {
+TEST_CONSTEXPR_CXX20 void test(S& s) {
   typename S::size_type old_cap = s.capacity();
   S s0                          = s;
   s.shrink_to_fit();

[philnik777]

Currently, when the string shrink into the SSO buffer, the lifetime of the buffer hasn't begun before copying characters into it, so the subsequent copy operation raises UB and thus causes constant evaluation failure.

This is incorrect. It's not UB, since we know that CharT is an implicit lifetime type, so we start the lifetime when copying the elements.

LGTM with updated commit message.

[frederick-vs-ja]

Currently, when the string shrink into the SSO buffer, the lifetime of the buffer hasn't begun before copying characters into it, so the subsequent copy operation raises UB and thus causes constant evaluation failure.

This is incorrect. It's not UB, since we know that CharT is an implicit lifetime type, so we start the lifetime when copying the elements.

LGTM with updated commit message.

Yeah. Changed to the following. Is it OK enough?

Currently, when the string shrink into the SSO buffer, the __rep_.__s member isn't activated before the traits_type::copy call
yet, so the __get_short_pointer reads an inactive member and then causes constant evaluation failure.

[philnik777]

Currently, when the string shrink into the SSO buffer, the lifetime of the buffer hasn't begun before copying characters into it, so the subsequent copy operation raises UB and thus causes constant evaluation failure.

This is incorrect. It's not UB, since we know that CharT is an implicit lifetime type, so we start the lifetime when copying the elements.
LGTM with updated commit message.

Yeah. Changed to the following. Is it OK enough?

Currently, when the string shrink into the SSO buffer, the __rep_.__s member isn't activated before the traits_type::copy call
yet, so the __get_short_pointer reads an inactive member and then causes constant evaluation failure.

Does the error occur in __get_short_pointer? IIRC that should only return a pointer to an inactive member, not actually read it. That might not be allowed during constant evaluation either, I can't remember.

[frederick-vs-ja]

Currently, when the string shrink into the SSO buffer, the lifetime of the buffer hasn't begun before copying characters into it, so the subsequent copy operation raises UB and thus causes constant evaluation failure.

This is incorrect. It's not UB, since we know that CharT is an implicit lifetime type, so we start the lifetime when copying the elements.
LGTM with updated commit message.

Yeah. Changed to the following. Is it OK enough?

Currently, when the string shrink into the SSO buffer, the __rep_.__s member isn't activated before the traits_type::copy call
yet, so the __get_short_pointer reads an inactive member and then causes constant evaluation failure.

Does the error occur in __get_short_pointer? IIRC that should only return a pointer to an inactive member, not actually read it. That might not be allowed during constant evaluation either, I can't remember.

Oops, I was confused and wrong. The error can be reproduced here: https://godbolt.org/z/MjMsf488W.

so internal __builtin_memmove call writing to the buffer causes constant evaluation failure