tsan: detect use-after-free

[kcc1, 2012-05-17 10:51:27]

Great idea! 
But I don't like to hide the "freed" bit inside TID. 
can we make this bit more explicit?

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
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

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
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
File rtl/tsan_defs.h (right):

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
File rtl/tsan_report.cc (right):

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?

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
rtl/tsan_rtl.h:94: // will race with it.
Too hackish. I'd prefer a separate bit.

[dvyukov, 2012-05-17 11:01:04]

On Thu, May 17, 2012 at 2:51 PM, <kcc@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<ht...
> 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?
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://code...
> 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://coderev...
> File rtl/tsan_defs.h (right):
>
>
http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_defs.**h#newcode32<ht...
> 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://co...
> 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://coderevi...
> File rtl/tsan_rtl.h (right):
>
>
http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_rtl.h#**newcode94<htt...
> 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.

[kcc1, 2012-05-17 11:15:33]

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

> On Thu, May 17, 2012 at 2:51 PM, <kcc@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://code...
>> 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://coderev...
>> 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://co...
>> 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://coderevi...
>> File rtl/tsan_rtl.h (right):
>>
>>
http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_rtl.h#**newcode94<htt...
>> 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.
>
>

[dvyukov, 2012-05-17 12:32:12]

On 2012/05/17 11:15:33, kcc1 wrote:
> On Thu, May 17, 2012 at 3:01 PM, Dmitry Vyukov <mailto:dvyukov@google.com>
wrote:
> 
> > On Thu, May 17, 2012 at 2:51 PM, <mailto:kcc@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%3Chttp://co...>
> >> 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%3Chttp://coder...>
> >> 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%3Chttp://...>
> >> 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%3Chttp://codere...>
> >> File rtl/tsan_rtl.h (right):
> >>
> >>
>
http://codereview.appspot.com/**6214052/diff/1/rtl/tsan_rtl.h#**newcode94%3Ch...>
> >> rtl/tsan_rtl.h:94: // will race with it.
> >> Too hackish. I'd prefer a separate bit.
> >>
> >>
>
http://codereview.appspot.com/**6214052/%3Chttp://codereview.appspot.com/6214...>
> >>
> >
> > I am thinking how to refactor the bit.

Done. PTAL.

[kcc1, 2012-05-17 13:02:43]

Looks good! 
Please add comments describing this hack and committ.