ThreadSanitizer instrumentation pass

[dvyukov, 2012-01-15 13:16:09]

we will do initialization in preinit array, right?

I am not that aware of llvm internals, so the review is mostly in the form of
questions.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:87: else if
(isa<CallInst>(BI))
won't it handle our __tsan_read/write calls?

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:95: "__tsan_func_entry",
IRB.getVoidTy(), NULL));
the instrumentation must be __tsan_func_entry(__builtin_return_address(0)); we
won't be able to cheaply/reliably obtain it in runtime.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:105: bool
ThreadSanitizer::instrumentLoadOrStore(Instruction *I) {
what is the meaning of the return value?

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:107: int IsWrite =
isa<StoreInst>(*I);
bool?

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:114: uint32_t TypeSize =
TD->getTypeStoreSizeInBits(OrigTy);
how does it handle bit fields?

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: FuncName +=
itostr(TypeSize / 8);
doesn't it have to something along the lines of CHAR_BITS?

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: FuncName +=
itostr(TypeSize / 8);
is it guaranteed to yield only 1/2/4/8 sizes?

[glider, 2012-01-16 11:11:30]

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:43: static char ID;  // Pass
identification, replacement for typeid
Please add a period at the end of the comment

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:52:
INITIALIZE_PASS(ThreadSanitizer, "tsan",
I think you should use INITIALIZE_PASS_DEPENDENCY(TargetData) and put it inside
INITIALIZE_PASS_BEGIN/INITIALIZE_PASS_END

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:84: Res |=
instrumentLoadOrStore(BI);
Won't it invalidate the BasicBlock::iterator?
I remember some discussion with Nick about this, but I can't remember why this
is correct.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:87: else if
(isa<CallInst>(BI))
On 2012/01/15 13:16:09, dvyukov wrote:
> won't it handle our __tsan_read/write calls?

I think Kostya exploits the fact that the read/write calls are inserted before
the load/store instructions, thus they are not handled by this loop at all. I
however doubt this is a good thing to do, see above.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:95: "__tsan_func_entry",
IRB.getVoidTy(), NULL));
On 2012/01/15 13:16:09, dvyukov wrote:
> the instrumentation must be __tsan_func_entry(__builtin_return_address(0)); we
> won't be able to cheaply/reliably obtain it in runtime.

Unless we build with frame pointers enabled. We theoretically can afford this.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:105: bool
ThreadSanitizer::instrumentLoadOrStore(Instruction *I) {
On 2012/01/15 13:16:09, dvyukov wrote:
> what is the meaning of the return value?

True means that the function has modified the IR. That's a standard LLVM
convention

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:105: bool
ThreadSanitizer::instrumentLoadOrStore(Instruction *I) {
On 2012/01/15 13:16:09, dvyukov wrote:
> what is the meaning of the return value?

It denotes whether the function changes the IR. This is a standard convention in
LLVM.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: FuncName +=
itostr(TypeSize / 8);
On 2012/01/15 13:16:09, dvyukov wrote:
> is it guaranteed to yield only 1/2/4/8 sizes?

It should, because that's the size required to store that type on the target
platform.

[kcc, 2012-01-18 00:37:38]

PTAL

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:43: static char ID;  // Pass
identification, replacement for typeid
On 2012/01/16 11:11:30, glider wrote:
> Please add a period at the end of the comment

Done.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:52:
INITIALIZE_PASS(ThreadSanitizer, "tsan",
On 2012/01/16 11:11:30, glider wrote:
> I think you should use INITIALIZE_PASS_DEPENDENCY(TargetData) and put it
inside
> INITIALIZE_PASS_BEGIN/INITIALIZE_PASS_END

I can't find any other module that does this for TargetData.
(asan does not)

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:84: Res |=
instrumentLoadOrStore(BI);
On 2012/01/16 11:11:30, glider wrote:
> Won't it invalidate the BasicBlock::iterator?
> I remember some discussion with Nick about this, but I can't remember why this
> is correct.

Maybe. I introduced a temp vector (just like in asan)

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:87: else if
(isa<CallInst>(BI))
On 2012/01/15 13:16:09, dvyukov wrote:
> won't it handle our __tsan_read/write calls?

Maybe. Fixed.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:95: "__tsan_func_entry",
IRB.getVoidTy(), NULL));
On 2012/01/15 13:16:09, dvyukov wrote:
> the instrumentation must be __tsan_func_entry(__builtin_return_address(0)); we
> won't be able to cheaply/reliably obtain it in runtime.

Yea.... Done.
Now it calls __tsan_func_entry(Intrinsic::returnaddress(0)))

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:107: int IsWrite =
isa<StoreInst>(*I);
On 2012/01/15 13:16:09, dvyukov wrote:
> bool?

Done.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:114: uint32_t TypeSize =
TD->getTypeStoreSizeInBits(OrigTy);
On 2012/01/15 13:16:09, dvyukov wrote:
> how does it handle bit fields?

Bit fields do not appear here. They are lowered to regular loads/stores before
this.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: FuncName +=
itostr(TypeSize / 8);
On 2012/01/15 13:16:09, dvyukov wrote:
> doesn't it have to something along the lines of CHAR_BITS?
Maybe, but I don't know about such constant in LLVM. 
No one objected for this use of 8 in asan code.

http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: FuncName +=
itostr(TypeSize / 8);
On 2012/01/15 13:16:09, dvyukov wrote:
> is it guaranteed to yield only 1/2/4/8 sizes?
No! 
I added the same guard as we have in asan

[glider, 2012-01-18 07:47:04]

>
http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
> lib/Transforms/Instrumentation/ThreadSanitizer.cpp:52:
> INITIALIZE_PASS(ThreadSanitizer, "tsan",
> On 2012/01/16 11:11:30, glider wrote:
> > I think you should use INITIALIZE_PASS_DEPENDENCY(TargetData) and put it
> inside
> > INITIALIZE_PASS_BEGIN/INITIALIZE_PASS_END
> 
> I can't find any other module that does this for TargetData.
> (asan does not)
> 
Maybe that's because TargetData is usually initialized by someone else. But:
https://llvm.org/svn/llvm-project/polly/trunk/lib/Analysis/TempScopInfo.cpp
(other occurrences of INITIALIZE_PASS_DEPENDENCY(TargetData) on Google refer to
my TSan pass :))

[glider, 2012-01-18 08:03:46]

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: Value *FuncEntryFunc =
CurrentModule->getOrInsertFunction(
I haven't thought about this before, but getOrInsertFunction() might invalidate
the Module::iterator. WDYT?

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:119: for (size_t i = 0, n =
RetVec.size(); i < n; i++) {
I suggest to make the size_t loop variable increments consistent (both i++ or
++i)

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:140: // Ignore all unusual
sizes.
TypeSize is always a multiple of 8. Don't you want to handle unusual sizes by
splitting them into several accesses?

[kcc, 2012-01-18 18:49:12]

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:116: Value *FuncEntryFunc =
CurrentModule->getOrInsertFunction(
On 2012/01/18 08:03:46, glider wrote:
> I haven't thought about this before, but getOrInsertFunction() might
invalidate
> the Module::iterator. WDYT?
This works perfectly for asan

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:119: for (size_t i = 0, n =
RetVec.size(); i < n; i++) {
On 2012/01/18 08:03:46, glider wrote:
> I suggest to make the size_t loop variable increments consistent (both i++ or
> ++i)

Done.

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:140: // Ignore all unusual
sizes.
On 2012/01/18 08:03:46, glider wrote:
> TypeSize is always a multiple of 8. 
not sure if this is true. 
> Don't you want to handle unusual sizes by
> splitting them into several accesses?
No, at least not yet

[kcc, 2012-01-18 18:49:57]

On 2012/01/18 07:47:04, glider wrote:
> >
>
http://codereview.appspot.com/5545054/diff/1/lib/Transforms/Instrumentation/T...
> > lib/Transforms/Instrumentation/ThreadSanitizer.cpp:52:
> > INITIALIZE_PASS(ThreadSanitizer, "tsan",
> > On 2012/01/16 11:11:30, glider wrote:
> > > I think you should use INITIALIZE_PASS_DEPENDENCY(TargetData) and put it
> > inside
> > > INITIALIZE_PASS_BEGIN/INITIALIZE_PASS_END
> > 
> > I can't find any other module that does this for TargetData.
> > (asan does not)
> > 
> Maybe that's because TargetData is usually initialized by someone else. But:
> https://llvm.org/svn/llvm-project/polly/trunk/lib/Analysis/TempScopInfo.cpp
> (other occurrences of INITIALIZE_PASS_DEPENDENCY(TargetData) on Google refer
to
> my TSan pass :))

The current code was explicitly suggested by Nick. 
Let's see what will LLVM folks say on this code.

[ramosian.glider, 2012-01-18 20:00:03]

> On 2012/01/18 08:03:46, glider wrote:
> > TypeSize is always a multiple of 8. 
> not sure if this is true. 
It is, see
http://llvm.org/docs/doxygen/html/classllvm_1_1TargetData.html#a211295d79113e...
> > Don't you want to handle unusual sizes by
> > splitting them into several accesses?
> No, at least not yet
Ok, no need to hurry

[ramosian.glider, 2012-01-18 20:00:23]

LGTM

[dvyukov, 2012-01-19 06:05:01]

LGTM

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
File lib/Transforms/Instrumentation/ThreadSanitizer.cpp (right):

http://codereview.appspot.com/5545054/diff/6002/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:140: // Ignore all unusual
sizes.
On 2012/01/18 08:03:46, glider wrote:
> TypeSize is always a multiple of 8. Don't you want to handle unusual sizes by
> splitting them into several accesses?

Or perhaps just rounding down the size (6->4).