From f52466e162d1ae56b9982829b9aa87e0be37b99f Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Fri, 22 Dec 2023 02:25:39 +0100 Subject: [PATCH 1/3] [ASan][libc++] String annotations optimizations fix with lambda 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. --- libcxx/include/string | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libcxx/include/string b/libcxx/include/string index c676182fba8ba..03f6655bb1e76 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()) From ab9bff2bad4a3ac48930aea7452fcdc8786c12d2 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Wed, 27 Dec 2023 09:36:56 +0100 Subject: [PATCH 2/3] Remove a commnet --- libcxx/include/string | 3 --- 1 file changed, 3 deletions(-) diff --git a/libcxx/include/string b/libcxx/include/string index 03f6655bb1e76..a67693fd28c64 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -922,9 +922,6 @@ 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. - // 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); From 716e066f6a71283bd0d5a0335297fe435cb85c07 Mon Sep 17 00:00:00 2001 From: Advenam Tacet Date: Wed, 27 Dec 2023 09:44:00 +0100 Subject: [PATCH 3/3] Whitespace --- libcxx/include/string | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcxx/include/string b/libcxx/include/string index a67693fd28c64..e2be53eaee241 100644 --- a/libcxx/include/string +++ b/libcxx/include/string @@ -922,7 +922,7 @@ 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_([](basic_string &__s){ if(!__s.__is_long()) __s.__annotate_delete(); return std::move(__s.__r_);}(__str)) { + : __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())