[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:1865
+
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __annotate_short_string_check() const _NOEXCEPT {
+#if (!defined(_LIBCPP_HAS_NO_ASAN)) && (_LIBCPP_CLANG_VER >= 1600)
----------------
philnik wrote:
> It's not clear to me what is actually checks for. Is it to check whether to poison the short string? I don't really have a better name right now, maybe someone has a good idea.
Yes, short string is poisoned if and only if this function returns true.


================
Comment at: libcxx/include/string:1867
+#if (!defined(_LIBCPP_HAS_NO_ASAN)) && (_LIBCPP_CLANG_VER >= 1600)
+      return !__libcpp_is_constant_evaluated() && is_same<allocator_type, __default_allocator_type>::value;
+#else
----------------
philnik wrote:
> Shouldn't it be legal to just poison the first `sizeof(__rep)` bytes? If the allocator is empty, it isn't allowed to access the memory anyways, and otherwise it will be layed out after the internal string representation.
Good point, it actually should work. I will test it and think about some edge cases.


================
Comment at: libcxx/include/string:1777-1780
+      if (__is_long())
+        __annotate_delete<false>();
+      else
+        __annotate_delete<true>();
----------------
philnik wrote:
> AdvenamTacet wrote:
> > philnik wrote:
> > > Why do you do this special-casing here?
> > In some places, it is unknown if object is in long or short state, there it has to be runtime check. In other situations, it is known and we can pass that information in template. It's not only about (insignificant) optimization, but it may be necessary if bytes in object contain trash (eg. object in the middle of modification) and `__is_long` may return incorrect value.
> I don't think we should design the interface for this. It should just be `__annotate_delete(); do-whatever-operation; __annotate_new(size)`. That makes the interface simpler and avoids any problems with the invariants not holding. I don't think any function is designed for this (other than the setters of course). Any performance-gains are probably non-existant to not-worth-the-effort. Taking a quick look through the code I also couldn't find a case where the invariants don't hold.
That decision was made because it was necessary, but a lot changed since that code was written. I will test if it's still necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769