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

[Kostya Serebryany]

Thanks!
r150423.

On Mon, Feb 13, 2012 at 11:37 AM, Nick Lewycky <nlewycky at google.com> wrote:

> 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/bb638ef1/attachment.html>