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

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet marked 2 inline comments as done.
AdvenamTacet added inline comments.


================
Comment at: libcxx/include/string:1644-1655
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS void
+    __raw_copy_r(basic_string& __dst, const basic_string& __src) _NOEXCEPT {
+      __dst.__r_.first().__r = __src.__r_.first().__r;
+    }
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS void
+    __raw_copy_l(basic_string& __dst, const basic_string& __src) _NOEXCEPT {
+      __dst.__r_.first().__l = __src.__r_.first().__l;
----------------
philnik wrote:
> Two of these are redundant. I've uploaded D132951 to remove the differences. Also, would it be possible to annotate the special member functions of `__rep` instead? I think adding `_LIBCPP_HIDE_FROM_ABI _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS __rep(const __rep&) = default;` should do the trick.
> I think adding _LIBCPP_HIDE_FROM_ABI _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS __rep(const __rep&) = default; should do the trick.

No, because we can also write to poisoned memory.


================
Comment at: libcxx/include/string:1670-1672
+    // The following functions are no-ops outside of AddressSanitizer mode.
+    // We call annotatations only for the default Allocator because other allocators
+    // may not meet the AddressSanitizer alignment constraints.
----------------
philnik wrote:
> Couldn't we check whether the allocation satisfies the asan requirements? It looks like the overhead of checking pointer alignment seems quite manageable: https://godbolt.org/z/WeE7Wxx73.
We can and now (due to changes in D132522) we do. I hope the check should be safe to remove. I can do it, but then it will be different in std::vector. Do we want to do it now and not (for example) wait for another revision to remove it here and in std::vector?


================
Comment at: libcxx/include/string:1681
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __annotate_contiguous_container(
+        const void* __beg, const void* __end, const void* __old_mid, const void* __new_mid) const {
+      if (!__libcpp_is_constant_evaluated())
----------------
philnik wrote:
> The two characters really don't hurt anybody and makes it a lot nicer to read.
I agree, but everywhere else those arguments are called beg, I'm happy to use begin, if inconsistency is not a problem. Is it?


================
Comment at: libcxx/include/string:1692-1695
+      if (!__libcpp_is_constant_evaluated())
+        return __sanitizer_is_annotable(this, sizeof(*this));
+      else
+        return false;
----------------
philnik wrote:
> 
This code has different behavior and we cannot really remove `if` from here.


================
Comment at: libcxx/include/string:1714-1718
+          __annotate_contiguous_container(
+              std::__to_address(__get_long_pointer()),
+              std::__to_address(__get_long_pointer() + __get_long_cap() + 1),
+              std::__to_address(__get_long_pointer() + __get_long_cap() + 1),
+              std::__to_address(__get_long_pointer() + __current_size + 1));
----------------
philnik wrote:
> Could we use a common interface for this and the vector-equivalent? It would be nice if we could add a `__debug_utils/address_sanitizer.h` where we define `__annotate_contiguous_container()` and some CRTP empty base that defines the functions used here. AFAICT it should be possible to define all of these functions for the long string with the public API of both `vector` and `string`. Then we could simply add a function here with the same name to special-case the short string. I hope this would simplify the implementation here quite a bit. I'm not sure though, so tell me if there is some problem.
Possible that it's possible for long string, not sure tho. But can we leave it for future improvements outside of that patch?
I will think about potential problems. Future patch may be for both std::basic_string and std::vector and maybe std::deque, if we make it more general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769