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

[dvyukov at google.com]

On 2012/05/17 11:15:33, kcc1 wrote:
> On Thu, May 17, 2012 at 3:01 PM, Dmitry Vyukov
<mailto:dvyukov at google.com> wrote:

> > On Thu, May 17, 2012 at 2:51 PM, <mailto: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%3Chttp://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%3Chttp://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%3Chttp://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%3Chttp://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%3Chttp://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/%3Chttp://codereview.appspot.com/6214052/>
> >>
> >
> > I am thinking how to refactor the bit.

Done. PTAL.


http://codereview.appspot.com/6214052/