[TSAN] Add optional support for distinguishing volatiles

[melver]

Summary

Add support to optionally emit different instrumentation for accesses to
volatile variables. While the default TSAN runtime likely will never
require this feature, other runtimes for different environments that
have subtly different memory models or assumptions may require
distinguishing volatiles.

One such environment are OS kernels, where volatile is still used in
various places for various reasons, and often declare volatile to be
"safe enough" even in multi-threaded contexts. One such example is the
Linux kernel, which implements various synchronization primitives using
volatile (READ_ONCE(), WRITE_ONCE()). Here the Kernel Concurrency
Sanitizer (KCSAN) [1], is a runtime that uses TSAN instrumentation but
otherwise implements a very different approach to race detection from
TSAN.

While in the Linux kernel it is generally discouraged to use volatiles
explicitly, the topic will likely come up again, and we will eventually
need to distinguish volatile accesses [2]. The other use-case is
ignoring data races on specially marked variables in the kernel, for
example bit-flags (here we may hide 'volatile' behind a different name
such as 'no_data_race').

[1] https://github.com/google/ktsan/wiki/KCSAN
[2] https://lkml.kernel.org/r/CANpmjNOfXNE-Zh3MNP=-gmnhvKbsfUfTtWkyg_=[e-mail]

[dvyukov]

I am not opposed to adding this in general. Andrey did the same for KTSAN in gcc long time ago.
However, this feature will be present only in the newest clang and not in gcc. So what is the use story for kernel? Do we make KCSAN require the newest clang? I think majority of kernel developers use gcc.

[melver]

In D78554#1994478, @dvyukov wrote:

I am not opposed to adding this in general. Andrey did the same for KTSAN in gcc long time ago.
However, this feature will be present only in the newest clang and not in gcc. So what is the use story for kernel? Do we make KCSAN require the newest clang? I think majority of kernel developers use gcc.

I am planning to make the same change to gcc. And yes, it's a problem that it's only the newest versions that will support this. But, for now it's not urgent and no kernel dev is explicitly requesting this. But my feeling is that eventually we will need it, and we will be glad to have this option when that day comes.

[melver]

FYI the build failures seem entirely unrelated and look like LLDB is broken.

[dvyukov]

In D78554#1994518, @melver wrote:

In D78554#1994478, @dvyukov wrote:

I am not opposed to adding this in general. Andrey did the same for KTSAN in gcc long time ago.
However, this feature will be present only in the newest clang and not in gcc. So what is the use story for kernel? Do we make KCSAN require the newest clang? I think majority of kernel developers use gcc.

I am planning to make the same change to gcc. And yes, it's a problem that it's only the newest versions that will support this. But, for now it's not urgent and no kernel dev is explicitly requesting this. But my feeling is that eventually we will need it, and we will be glad to have this option when that day comes.

Ack. At the very least we will be able to use this on syzbot.

[dvyukov]

Any objections, anybody?

Marco, do you want me to land this?

[melver]

In D78554#1994740, @dvyukov wrote:

Any objections, anybody?

Marco, do you want me to land this?

Thanks! I don't think I have access to land.

[melver]

In D78554#1994841, @melver wrote:

In D78554#1994740, @dvyukov wrote:

Any objections, anybody?

Marco, do you want me to land this?

Thanks! I don't think I have access to land.

I see no further complaints. Please go ahead and land. (I also have a prototype patch for KCSAN which works. Now on to GCC...)

[dvyukov]

ninja check-tsan passed.
ninja check-all kills my self-isolation machine, so let's assume it works...
committed as: https://github.com/llvm/llvm-project/commit/5a2c31116f412c3b6888be361137efd705e05814

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp

 //===-- ThreadSanitizer.cpp - race detector -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer, a race detector.
static cl::opt<bool>  ClHandleCxxExceptions(
     cl::desc("Handle C++ exceptions (insert cleanup blocks for unwinding)"),
     cl::Hidden);
 static cl::opt<bool>  ClInstrumentAtomics(
     "tsan-instrument-atomics", cl::init(true),
     cl::desc("Instrument atomics"), cl::Hidden);
 static cl::opt<bool>  ClInstrumentMemIntrinsics(
     "tsan-instrument-memintrinsics", cl::init(true),
     cl::desc("Instrument memintrinsics (memset/memcpy/memmove)"), cl::Hidden);
+static cl::opt<bool>  ClDistinguishVolatile(
+    "tsan-distinguish-volatile", cl::init(false),
+    cl::desc("Emit special instrumentation for accesses to volatiles"),
+    cl::Hidden);
 
 STATISTIC(NumInstrumentedReads, "Number of instrumented reads");
 STATISTIC(NumInstrumentedWrites, "Number of instrumented writes");
 STATISTIC(NumOmittedReadsBeforeWrite,
           "Number of reads ignored due to following writes");
 STATISTIC(NumAccessesWithBadSize, "Number of accesses with bad size");
 STATISTIC(NumInstrumentedVtableWrites, "Number of vtable ptr writes");
 STATISTIC(NumInstrumentedVtableReads, "Number of vtable ptr reads");
private:
   FunctionCallee TsanIgnoreBegin;
   FunctionCallee TsanIgnoreEnd;
   // Accesses sizes are powers of two: 1, 2, 4, 8, 16.
   static const size_t kNumberOfAccessSizes = 5;
   FunctionCallee TsanRead[kNumberOfAccessSizes];
   FunctionCallee TsanWrite[kNumberOfAccessSizes];
   FunctionCallee TsanUnalignedRead[kNumberOfAccessSizes];
   FunctionCallee TsanUnalignedWrite[kNumberOfAccessSizes];
+  FunctionCallee TsanVolatileRead[kNumberOfAccessSizes];
+  FunctionCallee TsanVolatileWrite[kNumberOfAccessSizes];
+  FunctionCallee TsanUnalignedVolatileRead[kNumberOfAccessSizes];
+  FunctionCallee TsanUnalignedVolatileWrite[kNumberOfAccessSizes];
   FunctionCallee TsanAtomicLoad[kNumberOfAccessSizes];
   FunctionCallee TsanAtomicStore[kNumberOfAccessSizes];
   FunctionCallee TsanAtomicRMW[AtomicRMWInst::LAST_BINOP + 1]
                               [kNumberOfAccessSizes];
   FunctionCallee TsanAtomicCAS[kNumberOfAccessSizes];
   FunctionCallee TsanAtomicThreadFence;
   FunctionCallee TsanAtomicSignalFence;
   FunctionCallee TsanVptrUpdate;
for (size_t i = 0; i < kNumberOfAccessSizes; ++i) {
     SmallString<64> UnalignedReadName("__tsan_unaligned_read" + ByteSizeStr);
     TsanUnalignedRead[i] = M.getOrInsertFunction(
         UnalignedReadName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
 
     SmallString<64> UnalignedWriteName("__tsan_unaligned_write" + ByteSizeStr);
     TsanUnalignedWrite[i] = M.getOrInsertFunction(
         UnalignedWriteName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
 
+    SmallString<64> VolatileReadName("__tsan_volatile_read" + ByteSizeStr);
+    TsanVolatileRead[i] = M.getOrInsertFunction(
+        VolatileReadName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
+
+    SmallString<64> VolatileWriteName("__tsan_volatile_write" + ByteSizeStr);
+    TsanVolatileWrite[i] = M.getOrInsertFunction(
+        VolatileWriteName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
+
+    SmallString<64> UnalignedVolatileReadName("__tsan_unaligned_volatile_read" +
+                                              ByteSizeStr);
+    TsanUnalignedVolatileRead[i] = M.getOrInsertFunction(
+        UnalignedVolatileReadName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
+
+    SmallString<64> UnalignedVolatileWriteName(
+        "__tsan_unaligned_volatile_write" + ByteSizeStr);
+    TsanUnalignedVolatileWrite[i] = M.getOrInsertFunction(
+        UnalignedVolatileWriteName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy());
+
     Type *Ty = Type::getIntNTy(M.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     SmallString<32> AtomicLoadName("__tsan_atomic" + BitSizeStr + "_load");
     TsanAtomicLoad[i] =
         M.getOrInsertFunction(AtomicLoadName, Attr, Ty, PtrTy, OrdTy);
 
     SmallString<32> AtomicStoreName("__tsan_atomic" + BitSizeStr + "_store");
     TsanAtomicStore[i] = M.getOrInsertFunction(
if (!IsWrite && isVtableAccess(I)) {
     IRB.CreateCall(TsanVptrLoad,
                    IRB.CreatePointerCast(Addr, IRB.getInt8PtrTy()));
     NumInstrumentedVtableReads++;
     return true;
   }
   const unsigned Alignment = IsWrite
       ? cast<StoreInst>(I)->getAlignment()
       : cast<LoadInst>(I)->getAlignment();
+  const bool IsVolatile =
+      ClDistinguishVolatile && (IsWrite ? cast<StoreInst>(I)->isVolatile()
+                                        : cast<LoadInst>(I)->isVolatile());
   Type *OrigTy = cast<PointerType>(Addr->getType())->getElementType();
   const uint32_t TypeSize = DL.getTypeStoreSizeInBits(OrigTy);
   FunctionCallee OnAccessFunc = nullptr;
-  if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0)
+  if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0) {
+    if (IsVolatile)
+      OnAccessFunc = IsWrite ? TsanVolatileWrite[Idx] : TsanVolatileRead[Idx];
+    else
       OnAccessFunc = IsWrite ? TsanWrite[Idx] : TsanRead[Idx];
+  } else {
+    if (IsVolatile)
+      OnAccessFunc = IsWrite ? TsanUnalignedVolatileWrite[Idx]
+                             : TsanUnalignedVolatileRead[Idx];
     else
       OnAccessFunc = IsWrite ? TsanUnalignedWrite[Idx] : TsanUnalignedRead[Idx];
+  }
   IRB.CreateCall(OnAccessFunc, IRB.CreatePointerCast(Addr, IRB.getInt8PtrTy()));
   if (IsWrite) NumInstrumentedWrites++;
   else         NumInstrumentedReads++;
   return true;
 }
 
 static ConstantInt *createOrdering(IRBuilder<> *IRB, AtomicOrdering ord) {
   uint32_t v = 0;

llvm/test/Instrumentation/ThreadSanitizer/volatile.ll

+; RUN: opt < %s -tsan -tsan-distinguish-volatile -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-S128"
+
+define i16 @test_volatile_read2(i16* %a) sanitize_thread {
+entry:
+  %tmp1 = load volatile i16, i16* %a, align 2
+  ret i16 %tmp1
+}
+
+; CHECK-LABEL: define i16 @test_volatile_read2(i16* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i16* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_read2(i8* %1)
+; CHECK-NEXT:   %tmp1 = load volatile i16, i16* %a, align 2
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i16
+
+define i32 @test_volatile_read4(i32* %a) sanitize_thread {
+entry:
+  %tmp1 = load volatile i32, i32* %a, align 4
+  ret i32 %tmp1
+}
+
+; CHECK-LABEL: define i32 @test_volatile_read4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_read4(i8* %1)
+; CHECK-NEXT:   %tmp1 = load volatile i32, i32* %a, align 4
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i32
+
+define i64 @test_volatile_read8(i64* %a) sanitize_thread {
+entry:
+  %tmp1 = load volatile i64, i64* %a, align 8
+  ret i64 %tmp1
+}
+
+; CHECK-LABEL: define i64 @test_volatile_read8(i64* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i64* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_read8(i8* %1)
+; CHECK-NEXT:   %tmp1 = load volatile i64, i64* %a, align 8
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i64
+
+define i128 @test_volatile_read16(i128* %a) sanitize_thread {
+entry:
+  %tmp1 = load volatile i128, i128* %a, align 16
+  ret i128 %tmp1
+}
+
+; CHECK-LABEL: define i128 @test_volatile_read16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_read16(i8* %1)
+; CHECK-NEXT:   %tmp1 = load volatile i128, i128* %a, align 16
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i128
+
+define void @test_volatile_write2(i16* %a) sanitize_thread {
+entry:
+  store volatile i16 1, i16* %a, align 2
+  ret void
+}
+
+; CHECK-LABEL: define void @test_volatile_write2(i16* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i16* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_write2(i8* %1)
+; CHECK-NEXT:   store volatile i16 1, i16* %a, align 2
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_volatile_write4(i32* %a) sanitize_thread {
+entry:
+  store volatile i32 1, i32* %a, align 4
+  ret void
+}
+
+; CHECK-LABEL: define void @test_volatile_write4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_write4(i8* %1)
+; CHECK-NEXT:   store volatile i32 1, i32* %a, align 4
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_volatile_write8(i64* %a) sanitize_thread {
+entry:
+  store volatile i64 1, i64* %a, align 8
+  ret void
+}
+
+; CHECK-LABEL: define void @test_volatile_write8(i64* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i64* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_write8(i8* %1)
+; CHECK-NEXT:   store volatile i64 1, i64* %a, align 8
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_volatile_write16(i128* %a) sanitize_thread {
+entry:
+  store volatile i128 1, i128* %a, align 16
+  ret void
+}
+
+; CHECK-LABEL: define void @test_volatile_write16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_volatile_write16(i8* %1)
+; CHECK-NEXT:   store volatile i128 1, i128* %a, align 16
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+; Check unaligned volatile accesses
+
+define i32 @test_unaligned_read4(i32* %a) sanitize_thread {
+entry:
+  %tmp1 = load volatile i32, i32* %a, align 2
+  ret i32 %tmp1
+}
+
+; CHECK-LABEL: define i32 @test_unaligned_read4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_volatile_read4(i8* %1)
+; CHECK-NEXT:   %tmp1 = load volatile i32, i32* %a, align 2
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i32
+
+define void @test_unaligned_write4(i32* %a) sanitize_thread {
+entry:
+  store volatile i32 1, i32* %a, align 1
+  ret void
+}
+
+; CHECK-LABEL: define void @test_unaligned_write4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_volatile_write4(i8* %1)
+; CHECK-NEXT:   store volatile i32 1, i32* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+; Check that regular aligned accesses are unaffected
+
+define i32 @test_read4(i32* %a) sanitize_thread {
+entry:
+  %tmp1 = load i32, i32* %a, align 4
+  ret i32 %tmp1
+}
+
+; CHECK-LABEL: define i32 @test_read4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_read4(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i32, i32* %a, align 4
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i32
+
+define void @test_write4(i32* %a) sanitize_thread {
+entry:
+  store i32 1, i32* %a, align 4
+  ret void
+}
+
+; CHECK-LABEL: define void @test_write4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_write4(i8* %1)
+; CHECK-NEXT:   store i32 1, i32* %a, align 4
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void