[libcxx-commits] [PATCH] D132769: [2b/3][ASan][libcxx] std::basic_string annotations

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet added inline comments.


================
Comment at: libcxx/include/string:867
 #  endif
-      : __r_(std::move(__str.__r_)) {
+      : __r_( (__str.__annotate_delete(), std::move(__str.__r_)) ) {
     __str.__default_init();
----------------
AdvenamTacet wrote:
> philnik wrote:
> > The `__annotate_delete()` shouldn't be necessary here, right? The function is already marked `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS`, so it should be enough to call `__annotate_delete()` right before `__default_init()` if it's needed at all.
> Unfortunately not. Sometimes instrumentation is turned on here. I tried that kind of solution first.
> 
> During las year I saw a lot of inconsistent behavior of ASan with variable initialization in constructors (`... : var_name(value)`) and turning off instrumentations (`_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS`).
> 
> In that case, it's necessary  with `test_allocator` (and probably some other of non-zero size, but I didn't confirm). Sometimes that kind of initialization is instrumented, even when function is not.
> 
> If we move it to the first line of the functions body (`__r_ = std::move(__str.__r_);`), then you are right.
> And then, `__annotate_delete()` does not have to be called every time.
> 
> But with unpoisoning every time, `_LIBCPP_STRING_INTERNAL_MEMORY_ACCESS` seems to be not necessary.
But `__annotate_delete()` should be only called if `__str` is short. 


================
Comment at: libcxx/include/string:869
     __str.__default_init();
+    __str.__annotate_new(0);
     std::__debug_db_insert_c(this);
----------------
philnik wrote:
> This seems to be redundant. `__default_init()` already calls `__annotate_new(0)`.
It didn't, but it actually can. I implemented it, makes code cleaner.


================
Comment at: libcxx/include/string:1767-1770
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
+    allocator_type& __alloc() _NOEXCEPT { return __r_.second(); }
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS
+    const allocator_type& __alloc() const _NOEXCEPT { return __r_.second(); }
----------------
philnik wrote:
> What exactly is the problem here? The memory should never actually be accessed, right? `__r_.second()` should just return a reference to either
> 1. memory outside the poisoned area, or
> 2. a reference inside the poisoned area that will never actually be used in a meaningful way (because of EBO).
Right, fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132769/new/

https://reviews.llvm.org/D132769