[libcxx-commits] [PATCH] D132769: [2b/3][ASan][libcxx] std::basic_string annotations
Tacet via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Apr 19 03:29:29 PDT 2023
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
More information about the libcxx-commits
mailing list