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

[Tacet via Phabricator via libcxx-commits]

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


================
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
----------------
AdvenamTacet wrote:
> 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.
With minor changes, it works. I added it.


================
Comment at: libcxx/include/string:1777-1780
+      if (__is_long())
+        __annotate_delete<false>();
+      else
+        __annotate_delete<true>();
----------------
AdvenamTacet wrote:
> 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.
It is no longer necessary, I removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769