Tread TSan LLVM flags to driver: account for external flag values in TSan.

[alekseyshl]

Summary

Tread TSan LLVM flags to driver: account for external flag values in TSan.

[eugenis]

LGTM

[pcc]

Could/should these be function attributes rather than pass arguments?

[dvyukov]

Could/should these be function attributes rather than pass arguments?

The use that we have for these flags is related to ignoring atomics in a huge source file, applying attributes to all functions in that file is impractical.
We have similar compiler flags for other sanitizers as well.
We could add attributes along with flags, but we don't have any uses for that at the moment.

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

85–96

Do we need default parameter values here? I would expect that not.

114

I would make them const as they are initialized only in ctor and are not meant to change.

[dvyukov]

LGTM with nits

[pcc]

In D26460#591074, @dvyukov wrote:

We could add attributes along with flags, but we don't have any uses for that at the moment.

The use case I have in mind is to make the sanitizers work better with LTO by running the sanitizer passes at link time. That would require that we preserve the flag settings in bitcode. Granted this isn't how things work right now, but it may be worth making it easier to get there given that these are new flags.

[eugenis]

Attribute is definitely the right long term solution, but it would be more efficient to move all sanitizer options to attributes in bulk. It also adds overhead (possibly tiny) compared to the pass argument approach, so it only makes sense to do if we plan to move instrumentation passes to the link stage right away. I don't think we want to work on that right now.

So the question is, do we want this temporary implementation, or are we fine with -mllvm flags? Seeing as the code is already written, I'd prefer to land this change now.

[pcc]

There are a couple of intermediate steps towards making the flags you're introducing in D26461 work correctly with LTO:

1. teach the clang driver to understand the flags
2. thread the flags down to the pass via attributes

What I would suggest is that we could initially take step 1 by having the driver translate -fsanitize-thread-* flags to -mllvm flags for the frontend, and take step 2 in a separate change.

[alekseyshl]

• Rename params to match their functionality.

[alekseyshl]

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

85–96

Yes, we do. FunctionPass is expected to have a zero arg ctor (see PassSupport.h, callDefaultCtor).

[dvyukov]

LGTM on my side
I don't know what amount of work is changing flags to attributes.

[alekseyshl]

This patch is not necessary anymore, D26461 uses legacy -llvm flags now.

[eugenis]

r286670

include/llvm/Transforms/Instrumentation.h

 ModulePass *createAddressSanitizerModulePass(bool CompileKernel = false,
                                              bool Recover = false);
 
 // Insert MemorySanitizer instrumentation (detection of uninitialized reads)
 FunctionPass *createMemorySanitizerPass(int TrackOrigins = 0,
                                         bool Recover = false);
 
 // Insert ThreadSanitizer (race detection) instrumentation
-FunctionPass *createThreadSanitizerPass();
+FunctionPass *createThreadSanitizerPass(bool InstrumentMemoryAccess = true,
+                                        bool InstrumentFuncEntryExit = true,
+                                        bool InstrumentAtomics = true);
 
 // Insert DataFlowSanitizer (dynamic data flow analysis) instrumentation
 ModulePass *createDataFlowSanitizerPass(
     const std::vector<std::string> &ABIListFiles = std::vector<std::string>(),
     void *(*getArgTLS)() = nullptr, void *(*getRetValTLS)() = nullptr);
 
 // Options for EfficiencySanitizer sub-tools.
 struct EfficiencySanitizerOptions {

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

 
 static const char *const kTsanModuleCtorName = "tsan.module_ctor";
 static const char *const kTsanInitName = "__tsan_init";
 
 namespace {
 
 /// ThreadSanitizer: instrument the code in module to find races.
 struct ThreadSanitizer : public FunctionPass {
-  ThreadSanitizer() : FunctionPass(ID) {}
+  ThreadSanitizer(bool InstrumentMemoryAccess = true,
+                  bool InstrumentFuncEntryExit = true,
+                  bool InstrumentAtomics = true)
+      : FunctionPass(ID),
+        InstrumentMemoryAccess(
+            InstrumentMemoryAccess && ClInstrumentMemoryAccesses),
+        InstrumentFuncEntryExit(
+            InstrumentFuncEntryExit && ClInstrumentFuncEntryExit),
+        InstrumentAtomics(
+            InstrumentAtomics && ClInstrumentAtomics),
+        InstrumentMemIntrinsics(
+            InstrumentMemoryAccess && ClInstrumentMemIntrinsics) {}
   StringRef getPassName() const override;
   void getAnalysisUsage(AnalysisUsage &AU) const override;
   bool runOnFunction(Function &F) override;
   bool doInitialization(Module &M) override;
   static char ID;  // Pass identification, replacement for typeid.
 
  private:
   void initializeCallbacks(Module &M);
   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);
 
+  const bool InstrumentMemoryAccess;
+  const bool InstrumentFuncEntryExit;
+  const bool InstrumentAtomics;
+  const bool InstrumentMemIntrinsics;
   Type *IntptrTy;
   IntegerType *OrdTy;
   // Callbacks to run-time library are computed in doInitialization.
   Function *TsanFuncEntry;
   Function *TsanFuncExit;
   // Accesses sizes are powers of two: 1, 2, 4, 8, 16.
   static const size_t kNumberOfAccessSizes = 5;
   Function *TsanRead[kNumberOfAccessSizes];
INITIALIZE_PASS_END(
     false, false)
 
 StringRef ThreadSanitizer::getPassName() const { return "ThreadSanitizer"; }
 
 void ThreadSanitizer::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<TargetLibraryInfoWrapperPass>();
 }
 
-FunctionPass *llvm::createThreadSanitizerPass() {
-  return new ThreadSanitizer();
+FunctionPass *llvm::createThreadSanitizerPass(
+    bool InstrumentMemoryAccess, bool InstrumentFuncEntryExit,
+    bool InstrumentAtomics) {
+  return new ThreadSanitizer(
+      InstrumentMemoryAccess, InstrumentFuncEntryExit, InstrumentAtomics);
 }
 
 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(
for (auto &BB : F) {
     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.
-  if (ClInstrumentMemoryAccesses && SanitizeFunction)
+  if (InstrumentMemoryAccess && SanitizeFunction)
     for (auto Inst : AllLoadsAndStores) {
       Res |= instrumentLoadOrStore(Inst, DL);
     }
 
   // Instrument atomic memory accesses in any case (they can be used to
   // implement synchronization).
-  if (ClInstrumentAtomics)
+  if (InstrumentAtomics)
     for (auto Inst : AtomicAccesses) {
       Res |= instrumentAtomic(Inst, DL);
     }
 
-  if (ClInstrumentMemIntrinsics && SanitizeFunction)
+  if (InstrumentMemIntrinsics && SanitizeFunction)
     for (auto Inst : MemIntrinCalls) {
       Res |= instrumentMemIntrinsic(Inst);
     }
 
   // Instrument function entry/exit points if there were instrumented accesses.
-  if ((Res || HasCalls) && ClInstrumentFuncEntryExit) {
+  if ((Res || HasCalls) && InstrumentFuncEntryExit) {
     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) {
       IRBuilder<> IRBRet(RetInst);
       IRBRet.CreateCall(TsanFuncExit, {});