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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik 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;
----------------
AdvenamTacet wrote:
> 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.
Yes, and why would that break if we annotate special member functions instead?


================
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.
----------------
AdvenamTacet wrote:
> 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?
I think we should fix it here now and update `vector` in a later patch.


================
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())
----------------
AdvenamTacet wrote:
> 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?
I guess you mean in `vector`? IMO we should just use proper names here and update `vector` in another patch. That should remove the inconsistency. We also never use `beg` anywhere else in the code base AFAIK, so `vector` is actually inconsistent IMO.


================
Comment at: libcxx/include/string:1692-1695
+      if (!__libcpp_is_constant_evaluated())
+        return __sanitizer_is_annotable(this, sizeof(*this));
+      else
+        return false;
----------------
AdvenamTacet wrote:
> philnik wrote:
> > 
> This code has different behavior and we cannot really remove `if` from here.
Oh yeah you're right. But you changed the code now, so AFAICT it should now be `return !__libcpp_is_constant_evaluated()`. Also, could you move the `#if` into the function to avoid declaring it twice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769