[tsan] Replace mem intrinsics with calls to interceptors

[vitalybuka]

Summary

After https://reviews.llvm.org/rG463aa814182a23 tsan replaces llvm
intrinsics with calls to glibc functions. However this approach is
fragile, as slight changes in pipeline can return llvm intrinsics back.
In particular InstCombine can do that.

Msan/Asan already declare own version of these memory
functions for the similar purpose.

KCSAN, or anything that uses something else than compiler-rt, needs to
implement this callbacks.

[melver]

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

344

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.

[vitalybuka]

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

344

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

[melver]

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

344

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

I added that because some kernel folks were complaining about compiler generating mem*() calls in uninstrumented functions, and mem*() being instrumented.

But KASAN isn't ready yet, so we can't make it the default. The kernel will flip the flag when it's been implemented, otherwise we just break older kernel unnecessarily. Also for KASAN, we need to support gcc, which does not emit __asan_mem*, and likely won't any time soon. So with the command line option we can dynamically detect if the compiler supports the prefix or not and then produce slightly different ways of instrumenting mem*() functions.

For the ThreadSanitizer pass change, since we don't distinguish between kernel and user space, there's not much we can do then other than break older kernels. I will ask stable kernel maintainers to backport the eventual fix.

I support the consistent prefix, i.e. __asan_, __msan_, and __tsan_.

Since it requires a kernel fix for KCSAN anyway (cmdline option or not), I think we can leave out the configurable option.

[vitalybuka]

update

[vitalybuka]

new line

[melver]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

3050

I think this change is not intended?

[vitalybuka]

clang format

[thakis]

This breaks building on mac: http://45.33.8.238/macm1/43944/step_4.txt

Please take a look and revert for now if it takes a while to fix.

[vitalybuka]

Thanks, reverted.

[vitalybuka]

Fix COMPILER_RT_DEBUG and Darwin builds

[nickdesaulniers]

It sounds like this is breaking the Linux kernel's concurrency sanitizer (KCSAN): https://github.com/ClangBuiltLinux/linux/issues/1704 PTAL

[melver]

In D133268#3774785, @nickdesaulniers wrote:

It sounds like this is breaking the Linux kernel's concurrency sanitizer (KCSAN): https://github.com/ClangBuiltLinux/linux/issues/1704 PTAL

Yes, it's expected - I'll send a patch. Ultimately this also an improvement for KCSAN since we can now instrument mem*().

compiler-rt/lib/tsan/rtl/tsan.syms.extra

 __tsan_init
 __tsan_flush_memory
 __tsan_read*
 __tsan_write*
 __tsan_vptr*
 __tsan_func*
 __tsan_atomic*
 __tsan_java*
 __tsan_unaligned*
 __tsan_release
 __tsan_acquire
+__tsan_memcpy
+__tsan_memmove
+__tsan_memset
 __tsan_mutex_create
 __tsan_mutex_destroy
 __tsan_mutex_pre_lock
 __tsan_mutex_post_lock
 __tsan_mutex_pre_unlock
 __tsan_mutex_post_unlock
 __tsan_mutex_pre_signal
 __tsan_mutex_post_signal

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

 // 3. Then we switched to atomics+sched_yield. But this produced tons of tsan-
 //    visible events, which lead to "failed to restore stack trace" failures.
 // Note that no_sanitize_thread attribute does not turn off atomic interception
 // so attaching it to the function defined in user code does not help.
 // That's why we now have what we have.
 constexpr u32 kBarrierThreadBits = 10;
 constexpr u32 kBarrierThreads = 1 << kBarrierThreadBits;
 
-extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __tsan_testonly_barrier_init(
+extern "C" {
+
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_testonly_barrier_init(
     atomic_uint32_t *barrier, u32 num_threads) {
   if (num_threads >= kBarrierThreads) {
     Printf("barrier_init: count is too large (%d)\n", num_threads);
     Die();
   }
   // kBarrierThreadBits lsb is thread count,
   // the remaining are count of entered threads.
   atomic_store(barrier, num_threads, memory_order_relaxed);
 }
 
 static u32 barrier_epoch(u32 value) {
   return (value >> kBarrierThreadBits) / (value & (kBarrierThreads - 1));
 }
 
-extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __tsan_testonly_barrier_wait(
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_testonly_barrier_wait(
     atomic_uint32_t *barrier) {
   u32 old = atomic_fetch_add(barrier, kBarrierThreads, memory_order_relaxed);
   u32 old_epoch = barrier_epoch(old);
   if (barrier_epoch(old + kBarrierThreads) != old_epoch) {
     FutexWake(barrier, (1 << 30));
     return;
   }
   for (;;) {
     u32 cur = atomic_load(barrier, memory_order_relaxed);
     if (barrier_epoch(cur) != old_epoch)
       return;
     FutexWait(barrier, cur);
   }
 }
+
+void *__tsan_memcpy(void *dst, const void *src, uptr size) {
+  void *ctx;
+#if PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE
+  COMMON_INTERCEPTOR_MEMCPY_IMPL(ctx, dst, src, size);
+#else
+  COMMON_INTERCEPTOR_MEMMOVE_IMPL(ctx, dst, src, size);
+#endif
+}
+
+void *__tsan_memset(void *dst, int c, uptr size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_MEMSET_IMPL(ctx, dst, c, size);
+}
+
+void *__tsan_memmove(void *dst, const void *src, uptr size) {
+  void *ctx;
+  COMMON_INTERCEPTOR_MEMMOVE_IMPL(ctx, dst, src, size);
+}
+}

compiler-rt/lib/tsan/rtl/tsan_interface.h

 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write4_pc(void *addr, void *pc);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write8_pc(void *addr, void *pc);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write16_pc(void *addr, void *pc);
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_vptr_read(void **vptr_p);
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_vptr_update(void **vptr_p, void *new_val);
 
+SANITIZER_INTERFACE_ATTRIBUTE
+void *__tsan_memcpy(void *dest, const void *src, uptr count);
+SANITIZER_INTERFACE_ATTRIBUTE
+void *__tsan_memset(void *dest, int ch, uptr count);
+SANITIZER_INTERFACE_ATTRIBUTE
+void *__tsan_memmove(void *dest, const void *src, uptr count);
+
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_func_entry(void *call_pc);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_func_exit();
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_ignore_thread_begin();
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_ignore_thread_end();
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void *__tsan_external_register_tag(const char *object_type);

compiler-rt/test/tsan/Linux/double_race.cpp

int main() {
   memset(buf, 1, sizeof(buf));
   barrier_wait(&barrier);
   pthread_join(t, 0);
   return 0;
 }
 
 // CHECK: WARNING: ThreadSanitizer: data race
 // CHECK:   Write of size 8 at {{.*}} by thread T1:
-// CHECK:     #0 memset
+// CHECK:     #0 {{.*}}memset
 // CHECK:     #{{[12]}} Thread
 // CHECK-NOT: bad PC passed to __tsan_symbolize_external
 // CHECK-NOT: __sanitizer_report_error_summary
 // CHECK-NOT: WARNING: ThreadSanitizer: data race

compiler-rt/test/tsan/inlined_memcpy_race.cpp

int main() {
   pthread_create(&t[1], NULL, MemSetThread, x);
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   fprintf(stderr, "PASS\n");
   return 0;
 }
 
 // CHECK: WARNING: ThreadSanitizer: data race
-// CHECK:   #0 memset
+// CHECK:   #0 {{.*}}memset
 // CHECK:   #{{[12]}} MemSetThread
 // CHECK:  Previous write
-// CHECK:   #0 {{(memcpy|memmove)}}
+// CHECK:   #0 {{.*mem(cpy|move)}}
 // CHECK:   #{{[12]}} MemCpyThread

compiler-rt/test/tsan/inlined_memcpy_race2.cpp

int main() {
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
 
   fprintf(stderr, "PASS\n");
   return 0;
 }
 
 // CHECK: WARNING: ThreadSanitizer: data race
-// CHECK:   #0 memset
+// CHECK:   #0 {{.*}}memset
 // CHECK:   #{{[12]}} MemSetThread
 // CHECK:  Previous write
-// CHECK:   #0 {{(memcpy|memmove)}}
+// CHECK:   #0 {{.*mem(cpy|move)}}
 // CHECK:   #{{[12]}} MemMoveThread

compiler-rt/test/tsan/memcmp_race.cpp

int main() {
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   return 0;
 }
 
 // CHECK: addr=[[ADDR:0x[0-9,a-f]+]]
 // CHECK: WARNING: ThreadSanitizer: data race
 // CHECK:   Write of size 3 at [[ADDR]] by thread T2:
-// CHECK:     #0 {{(memcpy|memmove)}}
+// CHECK:     #0 {{.*mem(cpy|move)}}
 // CHECK:     #{{[12]}} Thread2
 // CHECK:   Previous read of size 1 at [[ADDR]] by thread T1:
-// CHECK:     #0 memcmp
+// CHECK:     #0 {{.*}}memcmp
 // CHECK:     #{{[12]}} Thread1

compiler-rt/test/tsan/memcpy_race.cpp

int main() {
   pthread_join(t[1], NULL);
   return 0;
 }
 
 // CHECK: addr1=[[ADDR1:0x[0-9,a-f]+]]
 // CHECK: addr2=[[ADDR2:0x[0-9,a-f]+]]
 // CHECK: WARNING: ThreadSanitizer: data race
 // CHECK:   Write of size 4 at [[ADDR1]] by thread T2:
-// CHECK:     #0 {{(memcpy|memmove)}}
+// CHECK:     #0 {{.*mem(cpy|move)}}
 // CHECK:     #{{[12]}} Thread2
 // CHECK:   Previous write of size 1 at [[ADDR2]] by thread T1:
-// CHECK:     #0 {{(memcpy|memmove)}}
+// CHECK:     #0 {{.*mem(cpy|move)}}
 // CHECK:     #{{[12]}} Thread1

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

void ThreadSanitizer::initialize(Module &M) {
   {
     AttributeList AL = Attr;
     AL = AL.addParamAttribute(M.getContext(), 0, Attribute::ZExt);
     TsanAtomicSignalFence = M.getOrInsertFunction("__tsan_atomic_signal_fence",
                                                   AL, IRB.getVoidTy(), OrdTy);
   }
 
   MemmoveFn =
-      M.getOrInsertFunction("memmove", Attr, IRB.getInt8PtrTy(),
+      M.getOrInsertFunction("__tsan_memmove", Attr, IRB.getInt8PtrTy(),
                             IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy);
   MemcpyFn =
-      M.getOrInsertFunction("memcpy", Attr, IRB.getInt8PtrTy(),
+      M.getOrInsertFunction("__tsan_memcpy", Attr, IRB.getInt8PtrTy(),
                             IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy);
   MemsetFn =
-      M.getOrInsertFunction("memset", Attr, IRB.getInt8PtrTy(),
+      M.getOrInsertFunction("__tsan_memset", Attr, IRB.getInt8PtrTy(),
                             IRB.getInt8PtrTy(), IRB.getInt32Ty(), IntptrTy);
 }
 
 static bool isVtableAccess(Instruction *I) {
   if (MDNode *Tag = I->getMetadata(LLVMContext::MD_tbaa))
     return Tag->isTBAAVtableAccess();
   return false;
 }

llvm/test/Instrumentation/ThreadSanitizer/tsan_basic.ll

 
 ; Check that tsan converts mem intrinsics back to function calls.
 
 define void @MemCpyTest(i8* nocapture %x, i8* nocapture %y) sanitize_thread {
 entry:
     tail call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 %x, i8* align 4 %y, i64 16, i1 false)
     ret void
 ; CHECK: define void @MemCpyTest
-; CHECK: call i8* @memcpy
+; CHECK: call i8* @__tsan_memcpy
 ; CHECK: ret void
 }
 
 define void @MemCpyInlineTest(i8* nocapture %x, i8* nocapture %y) sanitize_thread {
 entry:
     tail call void @llvm.memcpy.inline.p0i8.p0i8.i64(i8* align 4 %x, i8* align 4 %y, i64 16, i1 false)
     ret void
 ; CHECK: define void @MemCpyInlineTest
-; CHECK: call i8* @memcpy
+; CHECK: call i8* @__tsan_memcpy
 ; CHECK: ret void
 }
 
 define void @MemMoveTest(i8* nocapture %x, i8* nocapture %y) sanitize_thread {
 entry:
     tail call void @llvm.memmove.p0i8.p0i8.i64(i8* align 4 %x, i8* align 4 %y, i64 16, i1 false)
     ret void
 ; CHECK: define void @MemMoveTest
-; CHECK: call i8* @memmove
+; CHECK: call i8* @__tsan_memmove
 ; CHECK: ret void
 }
 
 define void @MemSetTest(i8* nocapture %x) sanitize_thread {
 entry:
     tail call void @llvm.memset.p0i8.i64(i8* align 4 %x, i8 77, i64 16, i1 false)
     ret void
 ; CHECK: define void @MemSetTest
-; CHECK: call i8* @memset
+; CHECK: call i8* @__tsan_memset
 ; CHECK: ret void
 }
 
 define void @MemSetInlineTest(i8* nocapture %x) sanitize_thread {
 entry:
     tail call void @llvm.memset.inline.p0i8.i64(i8* align 4 %x, i8 77, i64 16, i1 false)
     ret void
 ; CHECK: define void @MemSetInlineTest
-; CHECK: call i8* @memset
+; CHECK: call i8* @__tsan_memset
 ; CHECK: ret void
 }
 
 ; CHECK-LABEL: @SwiftError
 ; CHECK-NOT: __tsan_read
 ; CHECK-NOT: __tsan_write
 ; CHECK: ret
 define void @SwiftError(i8** swifterror) sanitize_thread {