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

[Nick Lewycky]

On 13 February 2012 09:58, Kostya Serebryany <kcc at google.com> wrote:

>
>
> On Fri, Feb 10, 2012 at 10:56 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>
>> Thanks Kostya! This looks nearly ready to me.
>>
>> +; CHECK-NEXT:   call void @__tsan_func_exit()
>> +; CHECK-ret:
>>
>> Err, that last one should just be "CHECK: ret void".
>>
>>
>> Index: lib/Transforms/**Instrumentation/**ThreadSanitizer.cpp
>> ==============================**==============================**=======
>> --- lib/Transforms/**Instrumentation/**ThreadSanitizer.cpp  (revision 0)
>> +++ lib/Transforms/**Instrumentation/**ThreadSanitizer.cpp  (revision 0)
>> @@ -0,0 +1,172 @@
>>
>> +//===-- ThreadSanitizer.cpp - race detector ---------------------*- C++
>> -*-===//
>>
>> No emacs mode marker on .cpp files, only on .h files.
>>
>> +  for (int i = 0; i < kNumberOfAccessSizes; i++) {
>>
>> Elsewhere in the file you use "++i" form. I suggest using it here too.
>>
>> +    TsanWrite[i] = M.getOrInsertFunction(**WriteName, IRB.getVoidTy(),
>> +                                        IRB.getInt8PtrTy(), NULL);
>>
>> Indentation of the second line.
>>
>> +    Value *ReturnAddress = IRB.CreateCall(
>> +        Intrinsic::getDeclaration(**CurrentModule,
>> Intrinsic::returnaddress),
>> +        IRB.getInt32(0));
>>
>> Please use F.getParent() instead of tracking CurrentModule.
>>
>> +bool ThreadSanitizer::**instrumentLoadOrStore(**Instruction *I) {
>> +  IRBuilder<> IRB(I);
>> +  bool IsWrite = isa<StoreInst>(*I);
>> +  Value *Addr = IsWrite
>> +      ? cast<StoreInst>(I)->**getPointerOperand()
>> +      : cast<LoadInst>(I)->**getPointerOperand();
>> +  Type *OrigPtrTy = Addr->getType();
>> +  Type *OrigTy = cast<PointerType>(OrigPtrTy)->**getElementType();
>>
>> The element type of a pointer operand to a load or store instruction may
>> also be a VectorType (whose elements are in turn pointers). The semantics
>> of this are a scatter/gather operation.
>>
>> You may decide to bail on those, but please don't crash. :)
>>
>>
> I've fixed all except for the last bullet.
> Could you please give an example where this may fail?
> We have the exact same logic in AddressSanitizer and I have not seen any
> failures so far.
>

My apologies, for some reason I thought we added support for:

define <4 x i8> @test(<4 x i8*> %vec_of_pointers) {
  %A = load <4 x i8*> %vec_of_pointers
  ret <4 x i8> %A
}

but we actually only added support for icmp and GEP of vector-of-pointer,
not load and store of them. Carry on!

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120213/0b59bfcb/attachment.html>