[libcxx-commits] [libcxx] [libc++] Refactor memory allocation in basic_string (PR #128423)

[via libcxx-commits]

PiJoules wrote:

The trace points to the initialization of `__rep_` in

```
      // Turning off ASan instrumentation for variable initialization with _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
      // does not work consistently during initialization of __r_, so we instead unpoison __str's memory manually first.
      // __str's memory needs to be unpoisoned only in the case where it's a short string.
      : __rep_([](basic_string& __s) -> decltype(__s.__rep_)&& {
          if (!__s.__is_long())
            __s.__annotate_delete();
          return std::move(__s.__rep_);
        }(__str)),
```

and the asm tickling this is

```
; ./../../prebuilt/third_party/clang/linux-x64/bin/../include/c++/v1/string:1030
  2c3241: ba 18 00 00 00                movl    $0x18, %edx
  2c3246: 48 89 df                      movq    %rbx, %rdi
  2c3249: 4c 89 f6                      movq    %r14, %rsi
  2c324c: e8 1f e2 1a 00                callq   0x471470 <__asan_memcpy at plt>
```

where `rdi` is `this` and `rsi` is `__s.__rep_`. We fail in the `__asan_memcpy` because that memory region is still annotated. To me this implies that `__s_` was a long string and its shadow was not cleared. I tried combing over the code to see if perhaps some annotation logic was moved around but it's kind of non-trivial to assert since a bunch of stuff was moved and not all the refactors seem kind of 1-to-1.

One thing that stands out is the comment above this ctor which implies that the constructor should not be instrumented but this was a workaround since ASan was instrumenting initialization for `__rep_` despite being marked with `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS`. Perhaps this workaround can be removed and this failure we're seeing is a false positive if re-adding `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS` to the ctor works now?

https://github.com/llvm/llvm-project/pull/128423