[llvm-commits] tsan: detect use-after-free (issue 6214052)

[Kostya Serebryany]

On Thu, May 17, 2012 at 3:01 PM, Dmitry Vyukov <dvyukov at google.com> wrote:

> On Thu, May 17, 2012 at 2:51 PM, <kcc at google.com> wrote:
>
>> Great idea!
>> But I don't like to hide the "freed" bit inside TID.
>> can we make this bit more explicit?
>>
>
> I think yes. I am not very happy with current variant as well.
>
>
>>
>>
>> http://codereview.appspot.com/**6214052/diff/1/output_tests/**
>> free_race2.c<http://codereview.appspot.com/6214052/diff/1/output_tests/free_race2.c>
>> File output_tests/free_race2.c (right):
>>
>> http://codereview.appspot.com/**6214052/diff/1/output_tests/**
>> free_race2.c#newcode3<http://codereview.appspot.com/6214052/diff/1/output_tests/free_race2.c#newcode3>
>> output_tests/free_race2.c:3: void foo(int *mem) {
>> you will need to disable inlining.
>> This can be done later once we star running all tests with -O2
>>
>
> Do we actually need to run output tests with -finline?
>

We need to eventually run tests with -O2 (which implies inlining).
Our experience with asan shows that O1 and O2 are very different for this
kind of tools.
One of the reasons is that GVN (and load widening) is enabled only at O2.
One option is to run tests with -O2 -fno-inline, but I would prefer to mark
functions with noinline explicitly.

--kcc


> For functions for which it is irrelevant, it it irrelevant.
> For functions that require no-inline, we will need to add attributes.
> What's the point?
> I would prefer to add __attribute__((always_inline)) to inlining tests,
> if/when we have any. That will be required anyway, otherwise they will fail
> with -O0.
>
>
>
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_clock.**h<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_clock.h>
>> File rtl/tsan_clock.h (right):
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_clock.**
>> h#newcode75<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_clock.h#newcode75>
>> rtl/tsan_clock.h:75: u64 clk_[kMaxTid * 2];
>> Maybe we can use another kSomething?
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_defs.h<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_defs.h>
>> File rtl/tsan_defs.h (right):
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_defs.**
>> h#newcode32<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_defs.h#newcode32>
>> rtl/tsan_defs.h:32: const int kTidBits = 16;
>> I don't really like this.
>> Can we have a separate free-ed bit.
>> It can bit adjacent to tid so that the rest of the logic works.
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_**report.cc<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_report.cc>
>> File rtl/tsan_report.cc (right):
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_**
>> report.cc#newcode36<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_report.cc#newcode36>
>> rtl/tsan_report.cc:36: Printf("use after free");
>> maybe print "heap-use-after-free" to match asan's output?
>>
>
> Will do.
>
>
>
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_rtl.h<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_rtl.h>
>> File rtl/tsan_rtl.h (right):
>>
>> http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_rtl.h#**newcode94<http://codereview.appspot.com/6214052/diff/1/rtl/tsan_rtl.h#newcode94>
>> rtl/tsan_rtl.h:94: // will race with it.
>> Too hackish. I'd prefer a separate bit.
>>
>> http://codereview.appspot.com/**6214052/<http://codereview.appspot.com/6214052/>
>>
>
> I am thinking how to refactor the bit.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120517/0bb69691/attachment.html>