[ASan][libc++] Hotfix: String annotations stack frame size

[AdvenamTacet]

This change affects optimizations during compilation and reduces size of stack frames.

It's also one of many changes suggested in #76165. As #76082 was merged, I think that one should be as well.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Tacet (AdvenamTacet)

Changes

This change affects optimizations during compilation and reduces size of stack frames.

It's also one of many changes suggested in #76165. As #76082 was merged, I think that one should be as well.

---

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

1 Files Affected:

• (modified) libcxx/include/string (+6)

diff --git a/libcxx/include/string b/libcxx/include/string
index c676182fba8bac..f0af77c4d767ba 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -919,10 +919,16 @@ public:
 #  else
       _NOEXCEPT
 #  endif
+#ifndef _LIBCPP_HAS_NO_ASAN
       // 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_))) {
+    // ASan TODO: ^ This line results in stack frame growth and isn't correctly optimized by the compiler.
+    //              It should be refactored to improve performance.
+#else
+      : __r_(std::move(__str.__r_)) {
+#endif
     __str.__r_.first() = __rep();
     __str.__annotate_new(0);
     if (!__is_long())

[EricWF]

Please wait for @ldionne to approve this change.

[ldionne]

Under what circumstances is this not optimized? Do you have a godbolt link or something like that?

In general, we are comfortable relying on some optimizations to produce decent code. I am a bit shocked that this doesn’t behave like we’d expect in all cases, and if that’s the case we may want to rethink how much we expect from the compiler, but I’d like to do it based on solid evidence to avoid cargo-culting everywhere in the code. There’s a lot of places where we expect the compiler to perform similar optimizations, so in theory this kind of “hardcoding of simplifications” applies to a wide range of places.

[ldionne]

One could argue that the other asan ifdef patch has the same dilemma, but the crucial difference IMO is that this is applied at the point of call of the function, whereas the other patch added ifdefs inside the function, making them less of a maintenance burden.

[AdvenamTacet]

I could be wrong or I cannot reproduce the result I had yesterday. I have an example showing that this code is not fully optimized out, but the stack frame is of the same size. The example: https://godbolt.org/z/GnY3zq13K (ugly one, because it shows the difference better than LLVM17 vs trunk).

This example compares old string implementation (no string annotations) with new string implementation (with changes from the PR already merged, but without changes from this PR). You can see different outputs.

If we apply this PR, we get another example (old string without changes, new string with this PR as well): https://godbolt.org/z/av8cYd3Ts
You can see that outputs are the same.

In general, differences are not visible in trivial code examples, so the smallest example I could create showing some difference is:

std::string foo(std::string& s) {
    std::string s2(std::move(s));
    if(s2 == "x") {
        return s;
    } else {
        return s2;
    }
}

And you can see different output length for different sting versions without this PR. However, I cannot find an example showing different sizes of stack frames.

[AdvenamTacet]

Inspired by @ldionne, I created a lambda based solution #76200, which should be easier to maintain.