[llvm-commits] ThreadSanitizer, first patch. Please review.

[Nick Lewycky]

On 30 January 2012 09:45, Kostya Serebryany <kcc at google.com> wrote:

> Any feedback?


--- test/Instrumentation/ThreadSanitizer/tsan_basic.ll (revision 0)
+++ test/Instrumentation/ThreadSanitizer/tsan_basic.ll (revision 0)
@@ -0,0 +1,17 @@
+; 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 @read_4_bytes(i32* %a) {
+entry:
+  %tmp1 = load i32* %a, align 4
+  ret i32 %tmp1
+}
+; CHECK: @read_4_bytes
+; CHECK-NOT: ret i32
+; CHECK: __tsan_func_entry
+; CHECK: __tsan_read4
+; CHECK: __tsan_func_exit
+; CHECK: ret i32
+; CHECK: __tsan_init

I'm not a fan of this pile of CHECK statements. You could actually write
out the IR that you expect to get out and preserve the order using
CHECK-NEXT. Even if you don't want the matches to be too specific, it would
still be clearer to see "CHECK-NEXT: call {{.*}} @__tsan_func_entry".

Index: lib/Transforms/Instrumentation/ThreadSanitizer.cpp
===================================================================
--- lib/Transforms/Instrumentation/ThreadSanitizer.cpp (revision 0)
+++ lib/Transforms/Instrumentation/ThreadSanitizer.cpp (revision 0)
@@ -0,0 +1,149 @@
+//===-- ThreadSanitizer.cpp - race detector ---------------------*- C++
-*-===//

Hey hey, this ruler is 81 characters long. :)

+bool ThreadSanitizer::runOnModule(Module &M) {
+  bool Res = false;
+  CurrentModule = &M;
+  TD = getAnalysisIfAvailable<TargetData>();
+  if (!TD)
+    return false;
+
+  for (Module::iterator F = M.begin(), E = M.end(); F != E; ++F) {
+    if (F->isDeclaration()) continue;
+    Res |= handleFunction(M, *F);
+  }
+  // We instrumented at least one function. Insert a call to __tsan_init().
+  if (Res) {
+    IRBuilder<> IRB(M.getContext());
+    Value *TsanInit = M.getOrInsertFunction("__tsan_init",
+                                            IRB.getVoidTy(), NULL);
+    appendToGlobalCtors(M, cast<Function>(TsanInit), 0);
+  }
+  return Res;
+}

You could write this as a FunctionPass; create the __tsan_... methods in
doInitialization() (and store the necessary pointers in your object so that
you needn't look them up each time you instrument a function), then do the
instrumentation per-function, then in doFinalization either add the call to
__tsan_init or clean up declarations you created in doInitialization().

+      else if (isa<CallInst>(BI))
+        HasCalls = true;

What about invokes?

+  // Instrument memory accesses.
+  for (size_t i = 0, n = LoadsAndStores.size(); i < n; ++i) {
+    Res |= instrumentLoadOrStore(LoadsAndStores[i]);
+  }

Why not just call instrumentLoadOrStore as you encounter them? It seems
that the vector is just overhead.

Nick


> --kcc
>
>
>
> On Wed, Jan 18, 2012 at 11:38 AM, Kostya Serebryany <kcc at google.com>wrote:
>
>> Hello,
>>
>> The proposed patch is the first step towards race detection built into
>> LLVM/Clang.
>> The tool will be similar to AddressSanitizer in the way it works:
>>    1. A simple instrumentation module
>> in lib/Transforms/Instrumentation/ThreadSanitizer.cpp (this patch)
>>    2. -fthread-sanitizer flag in clang (next patch)
>>    3. A run-time library in projects/compiler-rt/lib/tsan (patches will
>> follow).
>>
>> The patch: http://codereview.appspot.com/5545054/ (also attached).
>>
>> Here are some links about the previous versions of ThreadSanitizer
>> - http://code.google.com/p/data-race-test/ -- main project page
>> - http://code.google.com/p/data-race-test/wiki/CompileTimeInstrumentation - description
>> of the LLVM-based prototype (we are not going to reuse that code, but will
>> reuse the ideas).
>> - http://code.google.com/p/data-race-test/wiki/GccInstrumentation -
>> description of the GCC-based prototype.
>> -
>> http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer -
>> ThreadSanitizer for Chromium
>> - http://data-race-test.googlecode.com/files/ThreadSanitizer.pdf --
>> paper about Valgrind-based tool published at WBIA'09
>> - http://data-race-test.googlecode.com/files/ThreadSanitizerLLVM.pdf --
>> paper about LLVM-based tool published at RV'2011
>>
>> Thanks,
>>
>> --kcc
>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120206/f0774ebb/attachment.html>