This is an archive of the discontinued LLVM Phabricator instance.

Differential D133268

[tsan] Replace mem intrinsics with calls to interceptors
ClosedPublic

Authored by vitalybuka on Sep 3 2022, 10:25 PM.

Details

Reviewers
dvyukov
melver
Commits
rGb4257d3bf58c: [tsan] Replace mem intrinsics with calls to interceptors
rG77654a65a373: [tsan] Replace mem intrinsics with calls to interceptors
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.

Diff Detail

Event Timeline

vitalybuka created this revision.Sep 3 2022, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 10:25 PM
Herald added subscribers: Enna1, hiraditya. · View Herald Transcript
vitalybuka requested review of this revision.Sep 3 2022, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 10:25 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
vitalybuka added reviewers: dvyukov, melver.Sep 3 2022, 10:25 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka retitled this revision from [tsan] Replace mem intrinsics with calls to interceptor to [tsan] Replace mem intrinsics with calls to interceptors.Sep 3 2022, 10:33 PM
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka edited the summary of this revision. (Show Details)
vitalybuka edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B184993: Diff 457835.Sep 4 2022, 12:14 AM
melver requested changes to this revision.Sep 5 2022, 12:01 AM
melver added inline comments.
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().

  1. 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 revision now requires changes to proceed.Sep 5 2022, 12:01 AM
vitalybuka added a subscriber: glider.Sep 5 2022, 9:44 AM
vitalybuka added inline comments.
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.

  1. 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 added inline comments.Sep 5 2022, 10:29 AM
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 edited the summary of this revision. (Show Details)Sep 5 2022, 5:47 PM
vitalybuka updated this revision to Diff 458070.Sep 5 2022, 5:48 PM

update

Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 5:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka updated this revision to Diff 458071.Sep 5 2022, 5:48 PM

new line

Harbormaster completed remote builds in B185158: Diff 458071.Sep 5 2022, 6:28 PM
melver accepted this revision.Sep 5 2022, 11:52 PM
melver added inline comments.
compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
3050 ↗(On Diff #458071)

I think this change is not intended?

This revision is now accepted and ready to land.Sep 5 2022, 11:52 PM
vitalybuka updated this revision to Diff 458178.Sep 6 2022, 8:24 AM
vitalybuka marked an inline comment as done.

clang format

This revision was landed with ongoing or failed builds.Sep 6 2022, 8:25 AM
Closed by commit rG77654a65a373: [tsan] Replace mem intrinsics with calls to interceptors (authored by vitalybuka). · Explain Why
This revision was automatically updated to reflect the committed changes.
vitalybuka added a commit: rG77654a65a373: [tsan] Replace mem intrinsics with calls to interceptors.
thakis added a subscriber: thakis.Sep 6 2022, 8:50 AM

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.

Harbormaster completed remote builds in B185227: Diff 458178.Sep 6 2022, 9:27 AM
vitalybuka added a reverting change: rGc51a12d598e9: Revert "[tsan] Replace mem intrinsics with calls to interceptors".Sep 6 2022, 9:47 AM
vitalybuka reopened this revision.Sep 6 2022, 9:47 AM

Thanks, reverted.

This revision is now accepted and ready to land.Sep 6 2022, 9:47 AM
vitalybuka updated this revision to Diff 458253.Sep 6 2022, 1:03 PM

Fix COMPILER_RT_DEBUG and Darwin builds

This revision was landed with ongoing or failed builds.Sep 6 2022, 1:09 PM
Closed by commit rGb4257d3bf58c: [tsan] Replace mem intrinsics with calls to interceptors (authored by vitalybuka). · Explain Why
This revision was automatically updated to reflect the committed changes.
vitalybuka added a commit: rGb4257d3bf58c: [tsan] Replace mem intrinsics with calls to interceptors.
Harbormaster completed remote builds in B185279: Diff 458253.Sep 6 2022, 2:52 PM
nickdesaulniers added a subscriber: nickdesaulniers.Sep 7 2022, 9:07 AM

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

melver added a comment.Sep 7 2022, 10:00 AM
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*().

Revision Contents

PathSize
llvm/
lib/
Transforms/
Instrumentation/
6 lines
test/
Instrumentation/
ThreadSanitizer/
10 lines

Diff 457835

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

Show First 20 Lines • Show All 335 Lines • ▼ Show 20 Linesvoid ThreadSanitizer::initialize(Module &M) {
{ {
AttributeList AL = Attr; AttributeList AL = Attr;
AL = AL.addParamAttribute(M.getContext(), 0, Attribute::ZExt); AL = AL.addParamAttribute(M.getContext(), 0, Attribute::ZExt);
TsanAtomicSignalFence = M.getOrInsertFunction("__tsan_atomic_signal_fence", TsanAtomicSignalFence = M.getOrInsertFunction("__tsan_atomic_signal_fence",
AL, IRB.getVoidTy(), OrdTy); AL, IRB.getVoidTy(), OrdTy);
} }
MemmoveFn = MemmoveFn =
M.getOrInsertFunction("memmove", Attr, IRB.getInt8PtrTy(), M.getOrInsertFunction("__interceptor_memmove", Attr, IRB.getInt8PtrTy(),
melverUnsubmitted
Not Done
ReplyInline Actions

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().

  1. 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.
melver: This will cause problems for non-TSan runtimes, like KCSAN (on Linux, but also Fuchsia, FreeBSD…
vitalybukaAuthorUnsubmitted
Done
ReplyInline Actions

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.

  1. 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

vitalybuka: > This will cause problems for non-TSan runtimes, like KCSAN (on Linux, but also Fuchsia…
melverUnsubmitted
Not Done
ReplyInline Actions

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.

melver: > Also I noticed ClKasanMemIntrinCallbackPrefix in asan, I propose to make it default, and…
IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy); IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy);
MemcpyFn = MemcpyFn =
M.getOrInsertFunction("memcpy", Attr, IRB.getInt8PtrTy(), M.getOrInsertFunction("__interceptor_memcpy", Attr, IRB.getInt8PtrTy(),
IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy); IRB.getInt8PtrTy(), IRB.getInt8PtrTy(), IntptrTy);
MemsetFn = MemsetFn =
M.getOrInsertFunction("memset", Attr, IRB.getInt8PtrTy(), M.getOrInsertFunction("__interceptor_memset", Attr, IRB.getInt8PtrTy(),
IRB.getInt8PtrTy(), IRB.getInt32Ty(), IntptrTy); IRB.getInt8PtrTy(), IRB.getInt32Ty(), IntptrTy);
} }
static bool isVtableAccess(Instruction *I) { static bool isVtableAccess(Instruction *I) {
if (MDNode *Tag = I->getMetadata(LLVMContext::MD_tbaa)) if (MDNode *Tag = I->getMetadata(LLVMContext::MD_tbaa))
return Tag->isTBAAVtableAccess(); return Tag->isTBAAVtableAccess();
return false; return false;
} }
▲ Show 20 Lines • Show All 478 Lines • Show Last 20 Lines

llvm/test/Instrumentation/ThreadSanitizer/tsan_basic.ll

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