diff --git a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp --- a/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp +++ b/llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp @@ -19,7 +19,8 @@ //===----------------------------------------------------------------------===// #include "llvm/Transforms/Instrumentation/ThreadSanitizer.h" -#include "llvm/ADT/SmallPtrSet.h" +#include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" @@ -52,30 +53,36 @@ #define DEBUG_TYPE "tsan" -static cl::opt ClInstrumentMemoryAccesses( +static cl::opt ClInstrumentMemoryAccesses( "tsan-instrument-memory-accesses", cl::init(true), cl::desc("Instrument memory accesses"), cl::Hidden); -static cl::opt ClInstrumentFuncEntryExit( - "tsan-instrument-func-entry-exit", cl::init(true), - cl::desc("Instrument function entry and exit"), cl::Hidden); -static cl::opt ClHandleCxxExceptions( +static cl::opt + ClInstrumentFuncEntryExit("tsan-instrument-func-entry-exit", cl::init(true), + cl::desc("Instrument function entry and exit"), + cl::Hidden); +static cl::opt ClHandleCxxExceptions( "tsan-handle-cxx-exceptions", cl::init(true), cl::desc("Handle C++ exceptions (insert cleanup blocks for unwinding)"), cl::Hidden); -static cl::opt ClInstrumentAtomics( - "tsan-instrument-atomics", cl::init(true), - cl::desc("Instrument atomics"), cl::Hidden); -static cl::opt ClInstrumentMemIntrinsics( +static cl::opt ClInstrumentAtomics("tsan-instrument-atomics", + cl::init(true), + cl::desc("Instrument atomics"), + cl::Hidden); +static cl::opt ClInstrumentMemIntrinsics( "tsan-instrument-memintrinsics", cl::init(true), cl::desc("Instrument memintrinsics (memset/memcpy/memmove)"), cl::Hidden); -static cl::opt ClDistinguishVolatile( +static cl::opt ClDistinguishVolatile( "tsan-distinguish-volatile", cl::init(false), cl::desc("Emit special instrumentation for accesses to volatiles"), cl::Hidden); -static cl::opt ClInstrumentReadBeforeWrite( +static cl::opt ClInstrumentReadBeforeWrite( "tsan-instrument-read-before-write", cl::init(false), cl::desc("Do not eliminate read instrumentation for read-before-writes"), cl::Hidden); +static cl::opt ClCompoundReadBeforeWrite( + "tsan-compound-read-before-write", cl::init(false), + cl::desc("Emit special compound instrumentation for reads-before-writes"), + cl::Hidden); STATISTIC(NumInstrumentedReads, "Number of instrumented reads"); STATISTIC(NumInstrumentedWrites, "Number of instrumented writes"); @@ -101,15 +108,37 @@ /// ensures the __tsan_init function is in the list of global constructors for /// the module. struct ThreadSanitizer { + ThreadSanitizer() { + // Sanity check options and warn user. + if (ClInstrumentReadBeforeWrite && ClCompoundReadBeforeWrite) { + errs() + << "warning: Option -tsan-compound-read-before-write has no effect " + "when -tsan-instrument-read-before-write is set.\n"; + } + } + bool sanitizeFunction(Function &F, const TargetLibraryInfo &TLI); private: + // Internal Instruction wrapper that contains more information about the + // Instruction from prior analysis. + struct InstructionInfo { + // Instrumentation emitted for this instruction is for a compounded set of + // read and write operations in the same basic block. + static constexpr unsigned kCompoundRW = (1U << 0); + + explicit InstructionInfo(Instruction *Inst) : Inst(Inst) {} + + Instruction *Inst; + unsigned Flags = 0; + }; + void initialize(Module &M); - bool instrumentLoadOrStore(Instruction *I, const DataLayout &DL); + bool instrumentLoadOrStore(const InstructionInfo &II, const DataLayout &DL); bool instrumentAtomic(Instruction *I, const DataLayout &DL); bool instrumentMemIntrinsic(Instruction *I); void chooseInstructionsToInstrument(SmallVectorImpl &Local, - SmallVectorImpl &All, + SmallVectorImpl &All, const DataLayout &DL); bool addrPointsToConstantData(Value *Addr); int getMemoryAccessFuncIndex(Value *Addr, const DataLayout &DL); @@ -130,6 +159,8 @@ FunctionCallee TsanVolatileWrite[kNumberOfAccessSizes]; FunctionCallee TsanUnalignedVolatileRead[kNumberOfAccessSizes]; FunctionCallee TsanUnalignedVolatileWrite[kNumberOfAccessSizes]; + FunctionCallee TsanCompoundRW[kNumberOfAccessSizes]; + FunctionCallee TsanUnalignedCompoundRW[kNumberOfAccessSizes]; FunctionCallee TsanAtomicLoad[kNumberOfAccessSizes]; FunctionCallee TsanAtomicStore[kNumberOfAccessSizes]; FunctionCallee TsanAtomicRMW[AtomicRMWInst::LAST_BINOP + 1] @@ -268,6 +299,15 @@ TsanUnalignedVolatileWrite[i] = M.getOrInsertFunction( UnalignedVolatileWriteName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy()); + SmallString<64> CompoundRWName("__tsan_read_write" + ByteSizeStr); + TsanCompoundRW[i] = M.getOrInsertFunction( + CompoundRWName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy()); + + SmallString<64> UnalignedCompoundRWName("__tsan_unaligned_read_write" + + ByteSizeStr); + TsanUnalignedCompoundRW[i] = M.getOrInsertFunction( + UnalignedCompoundRWName, Attr, IRB.getVoidTy(), IRB.getInt8PtrTy()); + Type *Ty = Type::getIntNTy(M.getContext(), BitSize); Type *PtrTy = Ty->getPointerTo(); SmallString<32> AtomicLoadName("__tsan_atomic" + BitSizeStr + "_load"); @@ -402,34 +442,42 @@ // 'Local' is a vector of insns within the same BB (no calls between). // 'All' is a vector of insns that will be instrumented. void ThreadSanitizer::chooseInstructionsToInstrument( - SmallVectorImpl &Local, SmallVectorImpl &All, - const DataLayout &DL) { - SmallPtrSet WriteTargets; + SmallVectorImpl &Local, + SmallVectorImpl &All, const DataLayout &DL) { + DenseMap WriteTargets; // Map of addresses to index in All // Iterate from the end. for (Instruction *I : reverse(Local)) { - if (StoreInst *Store = dyn_cast(I)) { - Value *Addr = Store->getPointerOperand(); - if (!shouldInstrumentReadWriteFromAddress(I->getModule(), Addr)) - continue; - WriteTargets.insert(Addr); - } else { - LoadInst *Load = cast(I); - Value *Addr = Load->getPointerOperand(); - if (!shouldInstrumentReadWriteFromAddress(I->getModule(), Addr)) - continue; - if (!ClInstrumentReadBeforeWrite && WriteTargets.count(Addr)) { - // We will write to this temp, so no reason to analyze the read. - NumOmittedReadsBeforeWrite++; - continue; + const bool IsWrite = isa(*I); + Value *Addr = IsWrite ? cast(I)->getPointerOperand() + : cast(I)->getPointerOperand(); + + if (!shouldInstrumentReadWriteFromAddress(I->getModule(), Addr)) + continue; + + if (!IsWrite) { + const auto WriteEntry = WriteTargets.find(Addr); + if (!ClInstrumentReadBeforeWrite && WriteEntry != WriteTargets.end()) { + auto &WI = All[WriteEntry->second]; + // If we distinguish volatile accesses and if either the read or write + // is volatile, do not omit any instrumentation. + const bool AnyVolatile = + ClDistinguishVolatile && (cast(I)->isVolatile() || + cast(WI.Inst)->isVolatile()); + if (!AnyVolatile) { + // We will write to this temp, so no reason to analyze the read. + // Mark the write instruction as compound. + WI.Flags |= InstructionInfo::kCompoundRW; + NumOmittedReadsBeforeWrite++; + continue; + } } + if (addrPointsToConstantData(Addr)) { // Addr points to some constant data -- it can not race with any writes. continue; } } - Value *Addr = isa(*I) - ? cast(I)->getPointerOperand() - : cast(I)->getPointerOperand(); + if (isa(GetUnderlyingObject(Addr, DL)) && !PointerMayBeCaptured(Addr, true, true)) { // The variable is addressable but not captured, so it cannot be @@ -438,7 +486,14 @@ NumOmittedNonCaptured++; continue; } - All.push_back(I); + + // Instrument this instruction. + All.emplace_back(I); + if (IsWrite) { + // For read-before-write and compound instrumentation we only need one + // write target, and we can override any previous entry if it exists. + WriteTargets[Addr] = All.size() - 1; + } } Local.clear(); } @@ -479,7 +534,7 @@ if (F.hasFnAttribute(Attribute::Naked)) return false; initialize(*F.getParent()); - SmallVector AllLoadsAndStores; + SmallVector AllLoadsAndStores; SmallVector LocalLoadsAndStores; SmallVector AtomicAccesses; SmallVector MemIntrinCalls; @@ -514,8 +569,8 @@ // Instrument memory accesses only if we want to report bugs in the function. if (ClInstrumentMemoryAccesses && SanitizeFunction) - for (auto Inst : AllLoadsAndStores) { - Res |= instrumentLoadOrStore(Inst, DL); + for (const auto &II : AllLoadsAndStores) { + Res |= instrumentLoadOrStore(II, DL); } // Instrument atomic memory accesses in any case (they can be used to @@ -553,13 +608,12 @@ return Res; } -bool ThreadSanitizer::instrumentLoadOrStore(Instruction *I, +bool ThreadSanitizer::instrumentLoadOrStore(const InstructionInfo &II, const DataLayout &DL) { - IRBuilder<> IRB(I); - bool IsWrite = isa(*I); - Value *Addr = IsWrite - ? cast(I)->getPointerOperand() - : cast(I)->getPointerOperand(); + IRBuilder<> IRB(II.Inst); + const bool IsWrite = isa(*II.Inst); + Value *Addr = IsWrite ? cast(II.Inst)->getPointerOperand() + : cast(II.Inst)->getPointerOperand(); // swifterror memory addresses are mem2reg promoted by instruction selection. // As such they cannot have regular uses like an instrumentation function and @@ -570,9 +624,9 @@ int Idx = getMemoryAccessFuncIndex(Addr, DL); if (Idx < 0) return false; - if (IsWrite && isVtableAccess(I)) { - LLVM_DEBUG(dbgs() << " VPTR : " << *I << "\n"); - Value *StoredValue = cast(I)->getValueOperand(); + if (IsWrite && isVtableAccess(II.Inst)) { + LLVM_DEBUG(dbgs() << " VPTR : " << *II.Inst << "\n"); + Value *StoredValue = cast(II.Inst)->getValueOperand(); // StoredValue may be a vector type if we are storing several vptrs at once. // In this case, just take the first element of the vector since this is // enough to find vptr races. @@ -588,36 +642,46 @@ NumInstrumentedVtableWrites++; return true; } - if (!IsWrite && isVtableAccess(I)) { + if (!IsWrite && isVtableAccess(II.Inst)) { IRB.CreateCall(TsanVptrLoad, IRB.CreatePointerCast(Addr, IRB.getInt8PtrTy())); NumInstrumentedVtableReads++; return true; } - const unsigned Alignment = IsWrite - ? cast(I)->getAlignment() - : cast(I)->getAlignment(); - const bool IsVolatile = - ClDistinguishVolatile && (IsWrite ? cast(I)->isVolatile() - : cast(I)->isVolatile()); + + const unsigned Alignment = IsWrite ? cast(II.Inst)->getAlignment() + : cast(II.Inst)->getAlignment(); + const bool IsCompoundRW = + ClCompoundReadBeforeWrite && (II.Flags & InstructionInfo::kCompoundRW); + const bool IsVolatile = ClDistinguishVolatile && + (IsWrite ? cast(II.Inst)->isVolatile() + : cast(II.Inst)->isVolatile()); + assert((!IsVolatile || !IsCompoundRW) && "Compound volatile invalid!"); + Type *OrigTy = cast(Addr->getType())->getElementType(); const uint32_t TypeSize = DL.getTypeStoreSizeInBits(OrigTy); FunctionCallee OnAccessFunc = nullptr; if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0) { - if (IsVolatile) + if (IsCompoundRW) + OnAccessFunc = TsanCompoundRW[Idx]; + else if (IsVolatile) OnAccessFunc = IsWrite ? TsanVolatileWrite[Idx] : TsanVolatileRead[Idx]; else OnAccessFunc = IsWrite ? TsanWrite[Idx] : TsanRead[Idx]; } else { - if (IsVolatile) + if (IsCompoundRW) + OnAccessFunc = TsanUnalignedCompoundRW[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++; + if (IsCompoundRW || IsWrite) + NumInstrumentedWrites++; + if (IsCompoundRW || !IsWrite) + NumInstrumentedReads++; return true; } diff --git a/llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll b/llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll --- a/llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll +++ b/llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll @@ -1,5 +1,7 @@ -; RUN: opt < %s -tsan -S | FileCheck %s +; RUN: opt < %s -tsan -S | FileCheck --check-prefixes=CHECK,CHECK-OPT %s ; RUN: opt < %s -tsan -tsan-instrument-read-before-write -S | FileCheck %s --check-prefixes=CHECK,CHECK-UNOPT +; RUN: opt < %s -tsan -tsan-compound-read-before-write -S | FileCheck %s --check-prefixes=CHECK,CHECK-COMPOUND +; RUN: opt < %s -tsan -tsan-distinguish-volatile -tsan-compound-read-before-write -S | FileCheck %s --check-prefixes=CHECK,CHECK-COMPOUND-VOLATILE 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" @@ -10,10 +12,13 @@ store i32 %inc, i32* %ptr, align 4 ret void } -; CHECK: define void @IncrementMe -; CHECK-NOT: __tsan_read -; CHECK-UNOPT: __tsan_read -; CHECK: __tsan_write +; CHECK-LABEL: define void @IncrementMe +; CHECK-OPT-NOT: __tsan_read4 +; CHECK-COMPOUND-NOT: __tsan_read4 +; CHECK-UNOPT: __tsan_read4 +; CHECK-OPT: __tsan_write4 +; CHECK-UNOPT: __tsan_write4 +; CHECK-COMPOUND: __tsan_read_write4 ; CHECK: ret void define void @IncrementMeWithCallInBetween(i32* nocapture %ptr) nounwind uwtable sanitize_thread { @@ -25,10 +30,52 @@ ret void } -; CHECK: define void @IncrementMeWithCallInBetween -; CHECK: __tsan_read -; CHECK: __tsan_write +; CHECK-LABEL: define void @IncrementMeWithCallInBetween +; CHECK: __tsan_read4 +; CHECK: __tsan_write4 ; CHECK: ret void declare void @foo() +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 +; CHECK-COMPOUND-NOT: __tsan_read4 +; CHECK-COMPOUND-VOLATILE: __tsan_volatile_read4 +; CHECK-COMPOUND: __tsan_read_write4 +; CHECK-COMPOUND-VOLATILE: __tsan_write4 +; CHECK: ret void + +define void @VolatileStore(i32* nocapture %ptr) nounwind uwtable sanitize_thread { +entry: + %0 = load i32, i32* %ptr, align 4 + %inc = add nsw i32 %0, 1 + store volatile i32 %inc, i32* %ptr, align 4 + ret void +} +; CHECK-LABEL: define void @VolatileStore +; CHECK-COMPOUND-NOT: __tsan_read4 +; CHECK-COMPOUND-VOLATILE: __tsan_read4 +; CHECK-COMPOUND: __tsan_read_write4 +; CHECK-COMPOUND-VOLATILE: __tsan_volatile_write4 +; CHECK: ret void + +define void @VolatileBoth(i32* nocapture %ptr) nounwind uwtable sanitize_thread { +entry: + %0 = load volatile i32, i32* %ptr, align 4 + %inc = add nsw i32 %0, 1 + store volatile i32 %inc, i32* %ptr, align 4 + ret void +} +; CHECK-LABEL: define void @VolatileBoth +; CHECK-COMPOUND-NOT: __tsan_read4 +; CHECK-COMPOUND-VOLATILE: __tsan_volatile_read4 +; CHECK-COMPOUND: __tsan_read_write4 +; CHECK-COMPOUND-VOLATILE: __tsan_volatile_write4 +; CHECK: ret void +