ThreadSanitizer v2 atomic support

[dvyukov, 2012-04-24 13:12:44]

Please take a brief look. Does it look sane?
It's not completed version.

[glider, 2012-04-24 13:46:20]

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

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:71: virtual const char
*getPassName() const;
This should be a public one.

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:90: Value
*TsanAtomicLoad[kNumberOfAccessSizes];
s/Value */Constant */

In fact you may want to check that the result of getOrInsertFunction is a
Function*, not a bitcast (see
http://code.google.com/p/address-sanitizer/issues/detail?id=67 and
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation...)

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:136: OrdTy =
IRB.getInt32Ty();
I wonder if it is not better to use Int64Ty on x64.

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:239: if (LoadInst *LI =
dyn_cast<LoadInst>(I))
isa<LoadInst>

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:342: if (LoadInst *LI =
dyn_cast<LoadInst>(I)) {
if (isa<LoadInst>(I))

See also http://llvm.org/docs/ProgrammersManual.html#isa

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:365: (void)AI;
Do you need this line?

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:374: int
ThreadSanitizer::getFuncIndex(Value *Addr) {
getFuncIdx is perhaps not the best name for a function that returns an index in
an array of load/store wrappers. It is not clear which function does the index
belong to (e.g. that's certainly not __tsan_init or __tsan_func_exit)

[dvyukov, 2012-04-24 16:42:15]

PTAL

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

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:71: virtual const char
*getPassName() const;
On 2012/04/24 13:46:20, glider wrote:
> This should be a public one.

Done.

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:90: Value
*TsanAtomicLoad[kNumberOfAccessSizes];
On 2012/04/24 13:46:20, glider wrote:
> s/Value */Constant */
> 
> In fact you may want to check that the result of getOrInsertFunction is a
> Function*, not a bitcast (see
> http://code.google.com/p/address-sanitizer/issues/detail?id=67 and
>
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation...)

Done.

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:136: OrdTy =
IRB.getInt32Ty();
On 2012/04/24 13:46:20, glider wrote:
> I wonder if it is not better to use Int64Ty on x64.

Int32Ty is OK on IA-32.

http://codereview.appspot.com/6119045/diff/2001/lib/Transforms/Instrumentatio...
lib/Transforms/Instrumentation/ThreadSanitizer.cpp:239: if (LoadInst *LI =
dyn_cast<LoadInst>(I))
On 2012/04/24 13:46:20, glider wrote:
> isa<LoadInst>

The style guide says:
if (AllocationInst *AI = dyn_cast<AllocationInst>(Val)) {
  // ...
}

[glider, 2012-04-25 10:15:22]

LGTM