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

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet marked an inline comment as done.
AdvenamTacet added inline comments.


================
Comment at: libcxx/include/string:1838
+    // ASan: short string is poisoned if and only if this function returns true.
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __annotate_short_string_check() const _NOEXCEPT {
+      return _LIBCPP_SHORT_STRING_ANNOTATIONS_ALLOWED && !__libcpp_is_constant_evaluated();
----------------
ldionne wrote:
> Would `__annotate_short_string_check` be better called something like `__short_string_is_annotated()`?
I changed it to `__asan_short_string_is_annotated`, but I don't fully like those names. I added `asan` so it is clear what kind of annotations are [not] there.


================
Comment at: libcxx/include/string:2472
+  pointer __p;
+  size_type __old_size;
+  if (__is_long()) {
----------------
ldionne wrote:
> I feel like this function would be a lot simpler if we instead called `__annotate_contiguous_container` directly, no?
I do not think so, and I don't like the idea of calling `__annotate_contiguous_container` directly, but I did improve the function a lot. Hope it's ok now.


================
Comment at: libcxx/include/string:2567
+
+  if (__str_was_short && this != &__str)
+    __str.__annotate_shrink(__str_old_size);
----------------
ldionne wrote:
> This whole set of changes feels kind of clunky. Why don't we annotate delete and then annotate new instead? Wouldn't that get rid of the special `this` check?
No, it wouldn't. Here we are working on `__str` and not `*this`. We cannot `__annotate_delete` `__str`, if it's long because we would have to annotate whole string every time (big performance hit and stil check).

We also shouldn't call `__annotate_delete` on `*this` and `__str` if those are the same objects.

So, the check would be just earlier. Or maybe I don't see something here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769