[tsan][llvm] Implement the function attribute to disable TSan checking at run time

[zaks.anna]

Summary

This implements a function annotation that disables TSan checking for the function at run time. The benefit over attribute((no_sanitize("thread"))) is that the accesses within the callees will also be suppressed.

The motivation for this attribute is a guarantee given by the objective C language that the calls to the reference count decrement and object deallocation will be synchronized. To model this properly, we would need to intercept all ref count decrement calls (which are very common in ObjC due to use of ARC) and also every single message send. Instead, we propose to just ignore all accesses made from within dealloc at run time. The main downside is that this still does not introduce any synchronization, which means we might still report false positives if the code that relies on this synchronization is not executed from within dealloc. However, we have not seen this in practice so far and think these cases will be very rare.

(This problem is similar in nature to https://reviews.llvm.org/D21609; unfortunately, the same solution does not apply here.)

This patch depends on https://reviews.llvm.org/D25857 and https://reviews.llvm.org/D25859

[zaks.anna]

Remove references to 'dealloc' from the comments.

[dvyukov]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

390 ↗

(On Diff #75384)

Isn't this just "return F.hasFnAttribute("sanitize_thread_no_checking_at_run_time")"?
If you remove the Attribute::SanitizeThread attribute when adding this new attribute, then isNoCheckingAtRunTime will be called at only 1 place. So it will be simpler to just use "F.hasFnAttribute("sanitize_thread_no_checking_at_run_time")" instead of it.

393 ↗

(On Diff #75384)

This will go away if you remove the Attribute::SanitizeThread attribute when adding this new attribute.

410 ↗

(On Diff #75384)

Do you have any particular use case for noreturn calls? I don't think it will work as expected. We don't return from that function, not from this function. Consider that we longjmp solely within this function, we will still exit from this function and call TsanIgnoreEnd on normal return path.
Moreover, most of the functions that recursively call noreturn function are not noreturn themselves. So if we call a function that recursively calls longjmp, we will fail to call TsanIgnoreEnd.

[zaks.anna]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

410 ↗

(On Diff #75384)

One of my reduced test cases was calling 'exit()' inside dealloc, which lead to the runtime warning about mismatched ignores. This code fixes that case. Given that, I do not think it's likely that anyone would call a no-return function from dealloc.

I see that this fix will not work in all cases as you pointed out. Would you prefer removing it altogether or adding a comment explaining that it won't work in general?

[dvyukov]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

410 ↗

(On Diff #75384)

For exit() we probably should fix runtime to not complain in such case (when thread is terminated due to exit), because it is not related this functionality, user can write something along the following lines:

...
ANNOTATE_IGNORE_READS_BEGIN();
...
if (some_error) {

printf(...);
exit(1);

}
...
ANNOTATE_IGNORE_READS_END();
...

Producing a warning in such case does not look particularly useful.
However, when main returns with ignores enabled we should still produce a warning, because that means that the program potentially runs with ignores enabled all the time due to a mismatched BEGIN/END somewhere.

Then I would prefer to remove special handling of noreturn functions, because it's not working.

We could handle ignores the same way we handle shadow stack for noreturn functions. I.e. longjmp unwinds shadow stack and restores some other internal state, it could also restore ignore counter.

[zaks.anna]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

410 ↗

(On Diff #75384)

Makes sense! I'll remove the handling of noreturn from this patch.

[zaks.anna]

Addressed review comments.

[dvyukov]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

458 ↗

(On Diff #76646)

What are the chances that these functions don't contain any function calls?
If the chances are considerable, then I would prefix this on "Res || HasCalls" because there is nothing to ignore otherwise.

[zaks.anna]

Only add the run time ignores if the function contains calls. Since the function is not marked as sanitize_thread, we should not have any reports directly from the body of the function.

[dvyukov]

I guess this will conflict with Kuba's patch for exceptions. One of you needs to rebase before submitting. I.e. here we want to end ignores on unwind.

[zaks.anna]

I'll rebase after the exceptions patch lands.

llvm/trunk/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

private:
   bool instrumentLoadOrStore(Instruction *I, const DataLayout &DL);
   bool instrumentAtomic(Instruction *I, const DataLayout &DL);
   bool instrumentMemIntrinsic(Instruction *I);
   void chooseInstructionsToInstrument(SmallVectorImpl<Instruction *> &Local,
                                       SmallVectorImpl<Instruction *> &All,
                                       const DataLayout &DL);
   bool addrPointsToConstantData(Value *Addr);
   int getMemoryAccessFuncIndex(Value *Addr, const DataLayout &DL);
+  void InsertRuntimeIgnores(Function &F, SmallVector<Instruction*, 8> &RetVec);
 
   Type *IntptrTy;
   IntegerType *OrdTy;
   // Callbacks to run-time library are computed in doInitialization.
   Function *TsanFuncEntry;
   Function *TsanFuncExit;
+  Function *TsanIgnoreBegin;
+  Function *TsanIgnoreEnd;
   // Accesses sizes are powers of two: 1, 2, 4, 8, 16.
   static const size_t kNumberOfAccessSizes = 5;
   Function *TsanRead[kNumberOfAccessSizes];
   Function *TsanWrite[kNumberOfAccessSizes];
   Function *TsanUnalignedRead[kNumberOfAccessSizes];
   Function *TsanUnalignedWrite[kNumberOfAccessSizes];
   Function *TsanAtomicLoad[kNumberOfAccessSizes];
   Function *TsanAtomicStore[kNumberOfAccessSizes];

 
 void ThreadSanitizer::initializeCallbacks(Module &M) {
   IRBuilder<> IRB(M.getContext());
   // Initialize the callbacks.
   TsanFuncEntry = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
       "__tsan_func_entry", IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
   TsanFuncExit = checkSanitizerInterfaceFunction(
       M.getOrInsertFunction("__tsan_func_exit", IRB.getVoidTy(), nullptr));
+  TsanIgnoreBegin = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
+      "__tsan_ignore_thread_begin", IRB.getVoidTy(), nullptr));
+  TsanIgnoreEnd = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
+      "__tsan_ignore_thread_end", IRB.getVoidTy(), nullptr));
   OrdTy = IRB.getInt32Ty();
   for (size_t i = 0; i < kNumberOfAccessSizes; ++i) {
     const unsigned ByteSize = 1U << i;
     const unsigned BitSize = ByteSize * 8;
     std::string ByteSizeStr = utostr(ByteSize);
     std::string BitSizeStr = utostr(BitSize);
     SmallString<32> ReadName("__tsan_read" + ByteSizeStr);
     TsanRead[i] = checkSanitizerInterfaceFunction(M.getOrInsertFunction(
if (isa<AtomicRMWInst>(I))
     return true;
   if (isa<AtomicCmpXchgInst>(I))
     return true;
   if (isa<FenceInst>(I))
     return true;
   return false;
 }
 
+void ThreadSanitizer::InsertRuntimeIgnores(Function &F,
+                                         SmallVector<Instruction*, 8> &RetVec) {
+  IRBuilder<> IRB(F.getEntryBlock().getFirstNonPHI());
+  IRB.CreateCall(TsanIgnoreBegin);
+  for (auto RetInst : RetVec) {
+    IRBuilder<> IRB(RetInst);
+    IRB.CreateCall(TsanIgnoreEnd);
+  }
+}
+
 bool ThreadSanitizer::runOnFunction(Function &F) {
   // This is required to prevent instrumenting call to __tsan_init from within
   // the module constructor.
   if (&F == TsanCtorFunction)
     return false;
   initializeCallbacks(*F.getParent());
   SmallVector<Instruction*, 8> RetVec;
   SmallVector<Instruction*, 8> AllLoadsAndStores;
for (auto Inst : AtomicAccesses) {
       Res |= instrumentAtomic(Inst, DL);
     }
 
   if (ClInstrumentMemIntrinsics && SanitizeFunction)
     for (auto Inst : MemIntrinCalls) {
       Res |= instrumentMemIntrinsic(Inst);
     }
 
+  if (F.hasFnAttribute("sanitize_thread_no_checking_at_run_time")) {
+    assert(!F.hasFnAttribute(Attribute::SanitizeThread));
+    if (HasCalls)
+      InsertRuntimeIgnores(F, RetVec);
+  }
+
   // Instrument function entry/exit points if there were instrumented accesses.
   if ((Res || HasCalls) && ClInstrumentFuncEntryExit) {
     IRBuilder<> IRB(F.getEntryBlock().getFirstNonPHI());
     Value *ReturnAddress = IRB.CreateCall(
         Intrinsic::getDeclaration(F.getParent(), Intrinsic::returnaddress),
         IRB.getInt32(0));
     IRB.CreateCall(TsanFuncEntry, ReturnAddress);
     for (auto RetInst : RetVec) {

llvm/trunk/test/Instrumentation/ThreadSanitizer/sanitize-thread-no-checking.ll

+; RUN: opt < %s -tsan -S | FileCheck %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @"\01-[NoCalls dealloc]"(i32* %a) "sanitize_thread_no_checking_at_run_time" {
+entry:
+  %tmp1 = load i32, i32* %a, align 4
+  ret i32 %tmp1
+}
+
+; CHECK: define i32 @"\01-[NoCalls dealloc]"(i32* %a)
+; CHECK-NEXT: entry:
+; CHECK-NEXT:   %tmp1 = load i32, i32* %a, align 4
+; CHECK-NEXT:   ret i32 %tmp1
+
+declare void @"foo"()
+
+define i32 @"\01-[WithCalls dealloc]"(i32* %a) "sanitize_thread_no_checking_at_run_time" {
+entry:
+  %tmp1 = load i32, i32* %a, align 4
+  call void @foo()
+  ret i32 %tmp1
+}
+
+; CHECK: define i32 @"\01-[WithCalls dealloc]"(i32* %a)
+; CHECK-NEXT: entry:
+; CHECK-NEXT:   %0 = call i8* @llvm.returnaddress(i32 0)
+; CHECK-NEXT:   call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   call void @__tsan_ignore_thread_begin()
+; CHECK-NEXT:   %tmp1 = load i32, i32* %a, align 4
+; CHECK-NEXT:   call void @foo()
+; CHECK-NEXT:   call void @__tsan_ignore_thread_end()
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK-NEXT:   ret i32 %tmp1