[PATCH] D133268: [tsan] Replace mem intrinsics with calls to interceptors

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a subscriber: glider.
vitalybuka added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp:344
   MemmoveFn =
-      M.getOrInsertFunction("memmove", Attr, IRB.getInt8PtrTy(),
+      M.getOrInsertFunction("__interceptor_memmove", Attr, IRB.getInt8PtrTy(),
                             IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy);
----------------
melver wrote:
> This will cause problems for non-TSan runtimes, like KCSAN (on Linux, but also Fuchsia, FreeBSD, etc.).
> 
> I also don't understand why this happens in the first place, isn't ThreadSanitizer pass supposed to run after such transforms?
> 
> Some options in order of preference:
> 
> 1. Is there another way to suppress this "optimization" back to an intrinsic? InstCombine has some exceptions when not to touch a call: https://github.com/llvm/llvm-project/blob/c4a174be9190b20eff0334415b5db7b2e2e204aa/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L2808
> So the function calls could be 'notail' AFAIK (I think llvm/test/Transforms/InstCombine/memcpy-1.ll is missing a test for that, it's only testing 'musttail'). Or a new attribute that's added to tryOptimizeCall().
> 
> 2. Make the prefix configurable with an option, so we can just set it to "" or `"__tsan_"` in the kernel (because `__interceptor_` seems too generic). It might be nice to mirror what ASan and MSan does, and actually call them `__tsan_mem*`, but it requires a runtime change as well.
> This will cause problems for non-TSan runtimes, like KCSAN (on Linux, but also Fuchsia, FreeBSD, etc.).
> 
> I also don't understand why this happens in the first place, isn't ThreadSanitizer pass supposed to run after such transforms?

This is not happening yet. 

I want to move sanitizers to early stages. Now we have simplification -> optimization -> sanitizers. I believe it should be simplification -> sanitizers -> optimization.

There is no reasons, other than bugs like this, why we don't apply optimization on instrumented code. It will improve size by 10% (probably also performance, I didn't measure perf yet). I mostly focused on Msan/Asan but other will benefit as well.

Btw. with ThinLTO (maybe full LTO as well) optimization passes already happen after sanitizers anyway. We don't test/use it much, but it should miss races on mem instructions.

> 
> Some options in order of preference:
> 
> 1. Is there another way to suppress this "optimization" back to an intrinsic? InstCombine has some exceptions when not to touch a call: https://github.com/llvm/llvm-project/blob/c4a174be9190b20eff0334415b5db7b2e2e204aa/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp#L2808
> So the function calls could be 'notail' AFAIK (I think llvm/test/Transforms/InstCombine/memcpy-1.ll is missing a test for that, it's only testing 'musttail'). Or a new attribute that's added to tryOptimizeCall().

I don't like this approach as it will leak a lot of sanitizers into other passes.
Fixing passes for santizers should be the last resort, we should rather try to generate code which will not be broken by valid transformation. Here prefixes is an easy solution.

> 
> 2. Make the prefix configurable with an option, so we can just set it to "" or `"__tsan_"` in the kernel (because `__interceptor_` seems too generic). It might be nice to mirror what ASan and MSan does, and actually call them `__tsan_mem*`, but it requires a runtime change as well.

I can do that, but it will be an issue for kernel, as we will optimize them out.
I think prefix (everywhere) and proper callback in the way to go, like we don in Msan.

I don't insist on __interceptor_, just thought that maybe we can use existing API.

Also I noticed ClKasanMemIntrinCallbackPrefix in asan, I propose to make it default, and maybe the only behavior. 

CC @glider 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133268