<br><br><div class="gmail_quote">On Thu, May 17, 2012 at 3:01 PM, Dmitry Vyukov <span dir="ltr"><<a href="mailto:dvyukov@google.com" target="_blank">dvyukov@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div class="im">On Thu, May 17, 2012 at 2:51 PM,  <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Great idea!<br>
But I don't like to hide the "freed" bit inside TID.<br>
can we make this bit more explicit?<br></blockquote><div><br></div></div><div>I think yes. I am not very happy with current variant as well.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/output_tests/free_race2.c" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/output_tests/<u></u>free_race2.c</a><br>
File output_tests/free_race2.c (right):<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/output_tests/free_race2.c#newcode3" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/output_tests/<u></u>free_race2.c#newcode3</a><br>
output_tests/free_race2.c:3: void foo(int *mem) {<br>
you will need to disable inlining.<br>
This can be done later once we star running all tests with -O2<br></blockquote><div><br></div></div><div>Do we actually need to run output tests with -finline?</div></div></blockquote><div><br></div><div>We need to eventually run tests with -O2 (which implies inlining). </div>
<div>Our experience with asan shows that O1 and O2 are very different for this kind of tools. </div><div>One of the reasons is that GVN (and load widening) is enabled only at O2. </div><div>One option is to run tests with -O2 -fno-inline, but I would prefer to mark functions with noinline explicitly. </div>
<div><br></div><div>--kcc </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote"><div>For functions for which it is irrelevant, it it irrelevant.</div>

<div>For functions that require no-inline, we will need to add attributes.</div><div>What's the point?</div><div>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.</div>
<div class="im">
<div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_clock.h" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_clock.<u></u>h</a><br>
File rtl/tsan_clock.h (right):<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_clock.h#newcode75" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_clock.<u></u>h#newcode75</a><br>
rtl/tsan_clock.h:75: u64 clk_[kMaxTid * 2];<br>
Maybe we can use another kSomething?<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_defs.h" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_defs.h</a><br>
File rtl/tsan_defs.h (right):<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_defs.h#newcode32" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_defs.<u></u>h#newcode32</a><br>
rtl/tsan_defs.h:32: const int kTidBits = 16;<br>
I don't really like this.<br>
Can we have a separate free-ed bit.<br>
It can bit adjacent to tid so that the rest of the logic works.<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_report.cc" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_<u></u>report.cc</a><br>
File rtl/tsan_report.cc (right):<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_report.cc#newcode36" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_<u></u>report.cc#newcode36</a><br>
rtl/tsan_report.cc:36: Printf("use after free");<br>
maybe print "heap-use-after-free" to match asan's output?<br></blockquote><div><br></div></div><div>Will do.</div><div class="im"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_rtl.h" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_rtl.h</a><br>
File rtl/tsan_rtl.h (right):<br>
<br>
<a href="http://codereview.appspot.com/6214052/diff/1/rtl/tsan_rtl.h#newcode94" target="_blank">http://codereview.appspot.com/<u></u>6214052/diff/1/rtl/tsan_rtl.h#<u></u>newcode94</a><br>
rtl/tsan_rtl.h:94: // will race with it.<br>
Too hackish. I'd prefer a separate bit.<br>
<br>
<a href="http://codereview.appspot.com/6214052/" target="_blank">http://codereview.appspot.com/<u></u>6214052/</a><br>
</blockquote></div></div><br><div>I am thinking how to refactor the bit.</div><div><br></div>
</blockquote></div><br>