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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: mikhail.ramalho.

Could you give a really simple example of issues this allows catching? Like the "Hello world" for this new functionality. I think that would be useful for folks (at least for me) to understand concretely what we're gaining (I have no doubt we're gaining something, I just don't see it clearly in my mind).

I see a lot of added complexity in this patch. It's only a couple of `if`s here and there, but it's in some of the trickiest code and most used code in the library. And that code is already very much in need of refactoring. Honestly, that makes me worried and from the maintainability and ease-to-validate-correctness perspective, this is only making things worse. It may still be worth doing if we're gaining something massive in return, but there's a clear tradeoff here. Also, this area is:

1. Super tricky cause it touches critical and already crufty code that few people understand well
2. In active development: `constexpr`-ification recently, hardening work, the debug mode that we gotta probably rip out, and now this

All in all, I guess I wish there had been a design document/RFC explaining this stack of changes before starting to try to get code changes into the library. If there's one, please point it out to me and apologies for my ignorance.



================
Comment at: libcxx/include/__string/extern_template_lists.h:30
 // must never be removed from the stable list.
+#if !defined(_LIBCPP_HAS_NO_ASAN) && (_LIBCPP_CLANG_VER >= 1600)
+// TODO LLVM18: Remove the special-casing (_LIBCPP_CLANG_VER)
----------------
philnik wrote:
> AdvenamTacet wrote:
> > philnik wrote:
> > > Why is this required?
> > We don't really want to use external templates here. Those we cannot control.
> > 
> > Without that casing, non-instrumented functions may be used. This leads to false positives. With that change, during compilation, a version with instrumentation is created. I never had a problem of missing instrumentation after that change.
> > 
> > Now when I look at it, `&& (_LIBCPP_CLANG_VER >= 1600)` should be removed, same problem may happen with older LLVM.
> IMO the right thing here would be to have an instrumented library, since the same problem exists for any other externally defined function, but I guess I'm OK with this for now. @ldionne are you OK with this?
Using ASAN is an ABI affecting change, kind of like our debug mode. Doing this will only seem to make things work, but in reality it'll cause other problems. For example, if you take a string created inside the dylib (without ASAN) and you try to use it from your ASAN-compiled user program, I assume you'll run into issues, right?

The solution is (quite unfortunately) to have a different built library for that configuration, unless I missed something.


================
Comment at: libcxx/include/string:1855
+    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 bool __annotate_short_string_check() const _NOEXCEPT {
+#if !defined(_LIBCPP_HAS_NO_ASAN) && !defined(_LIBCPP_ASAN_ANNOTATE_ONLY_LONG) && (_LIBCPP_CLANG_VER >= 1600)
+      // TODO LLVM18: remove special case value
----------------
`_LIBCPP_ASAN_ANNOTATE_ONLY_LONG` adds yet another configuration option. We try not to add configuration options unless we absolutely need to, so I would remove it. If users need to specify whether to annotate short strings or not, IMO the feature is mis-designed. Users shouldn't even be aware of the fact that we have short strings and long strings and they are implemented differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769