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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

On a general note: I don't have a problem with formatting the code around the changes, but let's do that in an NFC patch if you want to do that. The actual changes are quite hard to find sometimes.



================
Comment at: libcxx/include/string:1829-1837
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_pointer __get_short_pointer() const _NOEXCEPT {
+      return pointer_traits<const_pointer>::pointer_to(__r_.first().__s.__data_[0]);
+    }
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pointer __get_pointer() _NOEXCEPT {
+      return __is_long() ? __get_long_pointer() : __get_short_pointer();
+    }
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 const_pointer __get_pointer() const _NOEXCEPT {
----------------
These functions seem to be unchanged. If that's the case, please remove the formatting changes. This patch is already quite large, no need to make it larger.


================
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)
----------------
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.


================
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
----------------
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.


================
Comment at: libcxx/include/string:1657-1666
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_STRING_INTERNAL_MEMORY_ACCESS void __zero() _NOEXCEPT {
+      // ASan annotations:
+      // __zero() may be called when object fields contain random data,
+      // therefore we force caller to handle whole object memory annotations,
+      // as only caller knows if object is already initialized and
+      // __annotate_new() can be safely called.
+      //
----------------
AdvenamTacet wrote:
> philnik wrote:
> > This is now unnecessary.
> I'm not sure if I understand. Could you explain?
`__zero` doesn't exist anymore in trunk.


================
Comment at: libcxx/include/string:1777-1780
+      if (__is_long())
+        __annotate_delete<false>();
+      else
+        __annotate_delete<true>();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769