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

[Tacet via Phabricator via libcxx-commits]

AdvenamTacet added inline comments.


================
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)
----------------
ldionne wrote:
> 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.
In general, ASan does accept false positives in situation when part of the program is compiled without ASan. That may happen with `std::vector` annotations already. But minimizing the risk of it is a good course of action.
I agree that a separate built is the goal solution. I think, it may work as a temporary solution and then be changed into a separate library built. What do you think?

But I understand if you disagree, just I'm also not sure what has to be changed here to add an ASan library build to llvm. What has to be done? Where should I look to learn it?
Please, let me know.


================
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
----------------
ldionne wrote:
> `_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.
It's not necessary, I added it after discussion about problems ChromeOS had as another way to turn off annotations more granularly. However, I believe that D145628 already answers that problem. Removed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769