[ASan][libc++] String annotations optimizations fix with lambda

[AdvenamTacet]

This commit addresses optimization and instrumentation challenges encountered within comma constructors.

1. _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS does not work in comma constructors.
2. Code inside comma constructors is not always correctly optimized. Problematic code examples:
   • : __r_(((__str.__is_long() ? 0 : (__str.__annotate_delete(), 0)), std::move(__str.__r_))) {
   • : __r_(__r_([&](){ if(!__s.__is_long()) __s.__annotate_delete(); return std::move(__s.__r_);}())) {

However, lambda with argument seems to be correctly optimized. This patch uses that fact.

Use of lambda based on idea from @ldionne.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Tacet (AdvenamTacet)

Changes

This commit addresses optimization and instrumentation challenges encountered within comma constructors.

1. _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS does not work in comma constructors.
2. Code inside comma constructors is not always correctly optimized. Problematic code examples:
   • : _r(((__str.__is_long() ? 0 : (__str.__annotate_delete(), 0)), std::move(__str._r))) {
   • : _r(_r(&{ if(!__s.__is_long()) __s.__annotate_delete(); return std::move(__s._r);}())) {

However, lambda with argument seems to be correctly optimized. This patch uses that fact.

Use of lambda based on idea from @ldionne.

---

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

1 Files Affected:

• (modified) libcxx/include/string (+4-1)

diff --git a/libcxx/include/string b/libcxx/include/string
index c676182fba8bac..03f6655bb1e76e 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -922,7 +922,10 @@ public:
       // 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.
-      : __r_(((__str.__is_long() ? 0 : (__str.__annotate_delete(), 0)), std::move(__str.__r_))) {
+      // Lambda is used because of optimization challenges encountered within comma constructors.
+      // Lambda with argument is correctly optimized, but it does not solve the problem with internal memory
+      // access macro.
+      : __r_([](basic_string &__s){ if(!__s.__is_long()) __s.__annotate_delete(); return std::move(__s.__r_);}(__str)) {
     __str.__r_.first() = __rep();
     __str.__annotate_new(0);
     if (!__is_long())

[AdvenamTacet]

This does not answer problem of _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS not working inside comma constructors and therefore we have additional write to shadow memory while using ASan. That's a small cost, which I think we can ignore.

Alternative is a function like that:

  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
  __compressed_pair<__rep, allocator_type> && __compressed_pair_id(basic_string &__str) {
    if (!__str.__is_long())
      __str.__annotate_delete();
    return std::move(__str.__r_);
  }

But I support lambda solution.

[ldionne]

I'm fine with this (with a nitpick about the comment, which I think doesn't add much). This is kind of just a cleaner way to write the same thing as before, so even better if it fixes an optimization problem.

If this were making the code harder to read or more complicated, I'd ask for at least a regression test but in this case we can pretend that it's just a slight "refactoring", even though it is motivated by fixing a regression. It would be good to have some kind of regression test is that's feasible, though.

[ldionne]

Since this change is motivated by issues reported by @EricWF, I think it would be relevant for Eric to stamp this change as well, to make sure it solves at least some of the problems he was seeing.

[AdvenamTacet]

It also makes the code easier to read. Code inside of the lambda is easy to read, instead of being a hack with comma operator.

[EricWF]

LGTM, but I would like to see some actual assembly before and after in the review thread. Could you provide a godbolt or diff of the assembly generated?

I found some other things unrelated to this review while looking at this, which I'll bring up offline.

[AdvenamTacet]

@EricWF the difference is the same as described here: #76192 (comment)
Basically asm goes back to the state from before any string annotations on know to me examples.

I can create dedicated examples to that patch on Monday.

I also see a big benefit in readability.

[EricWF]

Actually I'm happy to generate the examples myself.

You've won me on the readability argument alone.

[AdvenamTacet]

Difference between no string annotations and current head with flags -stdlib=libc++ -fsanitize=undefined -O3 -std=c++23 -fno-omit-frame-pointer from the previous post: https://godbolt.org/z/GnY3zq13K Notice that there is NO -fsanitize=address, so we compile without ASan.

Example (but not only) difference:

        mov     byte ptr [rdi], r15b
        movzx   eax, byte ptr [rbp - 33]
        mov     byte ptr [rdi + 1], al
        mov     eax, dword ptr [rbp - 40]
        mov     dword ptr [rdi + 2], eax
        movzx   eax, word ptr [rbp - 36]
        mov     word ptr [rdi + 6], ax
        mov     qword ptr [rdi + 8], r14
        mov     qword ptr [rdi + 16], rbx

is changed to

        mov     byte ptr [rbx], r14b
        movzx   eax, byte ptr [rbp - 33]
        mov     byte ptr [rbx + 1], al
        mov     eax, dword ptr [rbp - 40]
        mov     dword ptr [rbx + 2], eax
        movzx   eax, word ptr [rbp - 36]
        mov     word ptr [rbx + 6], ax
        mov     qword ptr [rbx + 8], r12
        mov     qword ptr [rbx + 16], rdi
        mov     byte ptr [rbp - 33], 0
        mov     dword ptr [rbp - 40], 0
        mov     word ptr [rbp - 36], 0
        xor     edi, edi
        xor     r15d, r15d
        test    r15b, r15b
        je      .LBB1_16

But when we apply this PR, there is no difference between ASm before annotations were added and the new one: https://godbolt.org/z/Mb4bKsoGW

[AdvenamTacet]

It looks like this PR made a buildbot fail.
https://lab.llvm.org/buildbot/#/builders/37/builds/29650

I'm trying to understand why.

Edit:
Looks like returned type is incorrect. The lambda does not return rvalue reference.

[AdvenamTacet]

I reverted this PR: 1ea7a56

I believe the correct line should have additionally a return type. Like:

    : __r_([](basic_string &__s) -> auto&& { if(!__s.__is_long()) __s.__annotate_delete(); return std::move(__s.__r_); }(__str)) {

PR with changed, but still untested version is here: #77394