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

Sat Jun 10 16:58:36 PDT 2023

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)
----------------
AdvenamTacet wrote:
> ldionne wrote:
> > AdvenamTacet wrote:
> > > 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.
> > I believe this means that `-fsanitize=address` would have to cause a different `libc++.dylib` to be linked. So for example, the driver would add e.g. `-l c++asan` instead of `-l c++`, and we would have to produce `libc++asan.dylib` in addition to `libc++.dylib`. That's not a small change though, since it seems like this might be the first time a sanitizer has an effect on the version of libc++ we have to link against. But it might be the right way to go (probably the only way to go, really). In practice, disabling extern templates like this is only going to give users the impression that things work and then they will run into issues.
> > 
> > CC @vitalybuka
> @vitalybuka do you have any suggestion how to implement it?
> 
> Btw. a separate dylib may help with issue described here: https://github.com/llvm/llvm-project/issues/62431 (not sure, tho).
Hey @ldionne,
could you tell me where I should look to modify it? I don't really know how to start working on that change and I believe, it's the only missing part after landing D148693. I would appreciate letting me know at what files I should look first.

Btw. it won't affect the issue mentioned above.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132769