Thanks for the review! <div>Please take another look. <br><br><div class="gmail_quote">On Mon, Feb 6, 2012 at 2:35 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">On 30 January 2012 09:45, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Any feedback? </blockquote><div><br></div><div><div>--- test/Instrumentation/ThreadSanitizer/tsan_basic.ll<span style="white-space:pre-wrap"> </span>(revision 0)</div><div>+++ test/Instrumentation/ThreadSanitizer/tsan_basic.ll<span style="white-space:pre-wrap"> </span>(revision 0)</div>
<div>@@ -0,0 +1,17 @@</div><div>+; RUN: opt < %s -tsan -S | FileCheck %s</div><div>+</div><div>+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"</div>
<div>+target triple = "x86_64-unknown-linux-gnu"</div><div>+</div><div>+define i32 @read_4_bytes(i32* %a) {</div><div>+entry:</div><div>+ %tmp1 = load i32* %a, align 4</div><div>+ ret i32 %tmp1</div><div>+}</div>
<div>+; CHECK: @read_4_bytes</div><div>+; CHECK-NOT: ret i32</div><div>+; CHECK: __tsan_func_entry</div><div>+; CHECK: __tsan_read4</div><div>+; CHECK: __tsan_func_exit</div><div>+; CHECK: ret i32</div><div>+; CHECK: __tsan_init</div>
</div><div><br></div><div>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".</div>
</div></blockquote><div>done </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div><br></div><div><div>Index: lib/Transforms/Instrumentation/ThreadSanitizer.cpp</div><div>===================================================================</div><div>--- lib/Transforms/Instrumentation/ThreadSanitizer.cpp<span style="white-space:pre-wrap"> </span>(revision 0)</div>
<div>+++ lib/Transforms/Instrumentation/ThreadSanitizer.cpp<span style="white-space:pre-wrap"> </span>(revision 0)</div><div>@@ -0,0 +1,149 @@</div><div>+//===-- ThreadSanitizer.cpp - race detector ---------------------*- C++ -*-===//</div>
</div><div><br></div><div>Hey hey, this ruler is 81 characters long. :)</div></div></blockquote><div><br></div><div>Hm? My editor says 80 (81 counting the '+' in the patch). </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><br></div><div><div>+bool ThreadSanitizer::runOnModule(Module &M) {</div><div>+ bool Res = false;</div><div>+ CurrentModule = &M;</div>
<div>+ TD = getAnalysisIfAvailable<TargetData>();</div><div>+ if (!TD)</div><div>+ return false;</div><div>+</div><div>+ for (Module::iterator F = M.begin(), E = M.end(); F != E; ++F) {</div><div>+ if (F->isDeclaration()) continue;</div>
<div>+ Res |= handleFunction(M, *F);</div><div>+ }</div><div>+ // We instrumented at least one function. Insert a call to __tsan_init().</div><div>+ if (Res) {</div><div>+ IRBuilder<> IRB(M.getContext());</div>
<div>+ Value *TsanInit = M.getOrInsertFunction("__tsan_init",</div><div>+ IRB.getVoidTy(), NULL);</div><div>+ appendToGlobalCtors(M, cast<Function>(TsanInit), 0);</div>
<div>+ }</div><div>+ return Res;</div><div>+}</div></div><div><br></div><div>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().</div>
</div></blockquote><div><br></div><div>Rewrote to use FunctionPass. </div><div>However I have questions:</div><div> - 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)? </div>
<div> - You suggest to call M->getOrInsertFunction in doInitialization. Bu that will require 5x2+2=12 class members that will decrease the readability. </div><div> Also, there is no guarantee that all these objects will ever be used, so this is a kind of pessimization. </div>
<div> Besides, if getOrInsertFunction is slow, isn't it better to make it faster, than to manually optimize calls? </div><div> 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? </div>
<div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div><br></div><div>+ else if (isa<CallInst>(BI))</div><div><div><div>+ HasCalls = true;</div></div></div><div><br></div></div></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div></div><div>What about invokes?</div><div><br></div></div></blockquote><div><br></div><div>Yea, sure. Done. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div></div><div><div><div>+ // Instrument memory accesses.</div>
<div>+ for (size_t i = 0, n = LoadsAndStores.size(); i < n; ++i) {</div><div>+ Res |= instrumentLoadOrStore(LoadsAndStores[i]);</div><div>+ }</div></div></div><div><br></div><div>Why not just call instrumentLoadOrStore as you encounter them? It seems that the vector is just overhead.</div>
</div></blockquote><div><br></div><div>*in future* we will need to analyze this array and remove some of its elements. </div><div>Now it just looks a bit cleaner to me. </div><div><br></div><div><br></div><div>--kcc</div>
</div></div>