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

Kostya Serebryany kcc at google.com
Mon Feb 6 19:10:06 PST 2012


Thanks for the review!
Please take another look.

On Mon, Feb 6, 2012 at 2:35 PM, Nick Lewycky <nlewycky at google.com> wrote:

> 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".
>
done


>
> 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. :)
>

Hm? My editor says 80 (81 counting the '+' in the patch).



>
> +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().
>

Rewrote to use FunctionPass.
However I have questions:
   - Suppose I want to count the number of transformed instructions
in runOnFunction and store the result in the ThreadSanitizer object. Do I
need to use locking or atomics (OMG)?
   - You suggest to call M->getOrInsertFunction in doInitialization. Bu
that will require 5x2+2=12 class members that will decrease the
readability.
    Also, there is no guarantee that all these objects will ever be used,
so this is a kind of pessimization.
    Besides, if getOrInsertFunction is slow, isn't it better to make it
faster, than to manually optimize calls?
    We can do a lazy init for these objects, but then again what is the
preferred way to do lazy init in the FunctionPass to avoid races?




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


> What about invokes?
>
>
Yea, sure. Done.


> +  // 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.
>

*in future* we will need to analyze this array and remove some of its
elements.
Now it just looks a bit cleaner to me.


--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120206/4f8e340d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: issue5630068_2001.diff
Type: text/x-patch
Size: 2855 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120206/4f8e340d/attachment.bin>


More information about the llvm-commits mailing list