[TSan] Do not compound reads and writes when separated by atomics

[melver]

Summary

Do not compound reads and writes to the same address when separated by
atomic operations, including fences.

[dvyukov]

What would be an example of code where it matters?

[melver]

In D83949#2155758, @dvyukov wrote:

What would be an example of code where it matters?

For the kernel it doesn't matter, because we don't use builtin atomics (yet). The asm case works because it looks like a call.

For normal code, you could have something like:

T0:
  x = load A  // data race
  atomic load acquire B // observes 1
  store A, ...

T1:
  store A, ...
  atomic store release B, 1

If the current logic compounds this and TSAN only sees:

T0:
  atomic load acquire B // observes 1
  store A, ...

we might miss the data race of the load on A. In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

[dvyukov]

In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

Yes, that's what I am thinking as well. This only makes difference for a ~1 instruction window for the context where everything is timing dependent anyway...

[melver]

In D83949#2155877, @dvyukov wrote:

In practice, it probably doesn't really matter because T0 would probably load acquire spin on B or something better, and it'd be all in separate basic blocks.

Yes, that's what I am thinking as well. This only makes difference for a ~1 instruction window for the context where everything is timing dependent anyway...

Should we drop this? I think unless Alex has objections, I think it's fine to drop. I certainly don't feel too strongly either way.

[glider]

I don't object against dropping it. I had another case in mind, which turned out to be an actual data race, not something that the compiler pulled out of thin air.

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

bool ThreadSanitizer::sanitizeFunction(Function &F,
   bool Res = false;
   bool HasCalls = false;
   bool SanitizeFunction = F.hasFnAttribute(Attribute::SanitizeThread);
   const DataLayout &DL = F.getParent()->getDataLayout();
 
   // Traverse all instructions, collect loads/stores/returns, check for calls.
   for (auto &BB : F) {
     for (auto &Inst : BB) {
-      if (isAtomic(&Inst))
+      bool Barrier = false;
+
+      if (isAtomic(&Inst)) {
         AtomicAccesses.push_back(&Inst);
-      else if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
+        Barrier = true;
+      } else if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst)) {
         LocalLoadsAndStores.push_back(&Inst);
-      else if (isa<CallInst>(Inst) || isa<InvokeInst>(Inst)) {
+      } else if (isa<CallInst>(Inst) || isa<InvokeInst>(Inst)) {
         if (CallInst *CI = dyn_cast<CallInst>(&Inst))
           maybeMarkSanitizerLibraryCallNoBuiltin(CI, &TLI);
         if (isa<MemIntrinsic>(Inst))
           MemIntrinCalls.push_back(&Inst);
         HasCalls = true;
+        Barrier = true;
+      }
+
+      if (Barrier)
+        // Flush instructions found so far, avoiding compounding instrumentation
+        // with any instructions that follow.
         chooseInstructionsToInstrument(LocalLoadsAndStores, AllLoadsAndStores,
                                        DL);
-      }
-    }
     chooseInstructionsToInstrument(LocalLoadsAndStores, AllLoadsAndStores, DL);
   }
 
   // We have collected all loads and stores.
   // FIXME: many of these accesses do not need to be checked for races
   // (e.g. variables that do not escape, etc).
 
   // Instrument memory accesses only if we want to report bugs in the function.

llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll

 
 ; CHECK-LABEL: define void @IncrementMeWithCallInBetween
 ; CHECK: __tsan_read4
 ; CHECK: __tsan_write4
 ; CHECK: ret void
 
 declare void @foo()
 
+define void @IncrementWithFence(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
+entry:
+  %0 = load i32, i32* %ptr, align 4
+  %inc = add nsw i32 %0, 1
+  fence seq_cst
+  store i32 %inc, i32* %ptr, align 4
+  ret void
+}
+
+; CHECK-LABEL: define void @IncrementWithFence
+; CHECK: __tsan_read4
+; CHECK: __tsan_atomic_thread_fence
+; CHECK: __tsan_write4
+; CHECK: ret void
+
+define void @IncrementWithAtomic(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
+entry:
+  %0 = load i32, i32* %ptr, align 4
+  %inc = add nsw i32 %0, 1
+  %1 = load atomic i32, i32* %ptr acquire, align 4
+  store i32 %inc, i32* %ptr, align 4
+  ret void
+}
+
+; CHECK-LABEL: define void @IncrementWithAtomic
+; CHECK: __tsan_read4
+; CHECK: __tsan_atomic32_load
+; CHECK: __tsan_write4
+; CHECK: ret void
+
+define void @IncrementWithAsm(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
+entry:
+  %0 = load i32, i32* %ptr, align 4
+  %inc = add nsw i32 %0, 1
+  call void asm sideeffect "", ""()
+  store i32 %inc, i32* %ptr, align 4
+  ret void
+}
+
+; CHECK-LABEL: define void @IncrementWithAsm
+; CHECK: __tsan_read4
+; CHECK: __tsan_write4
+; CHECK: ret void
+
 define void @VolatileLoad(i32* nocapture %ptr) nounwind uwtable sanitize_thread {
 entry:
   %0 = load volatile i32, i32* %ptr, align 4
   %inc = add nsw i32 %0, 1
   store i32 %inc, i32* %ptr, align 4
   ret void
 }
 ; CHECK-LABEL: define void @VolatileLoad