<div dir="ltr"><div class="gmail_default" style><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 9, 2013 at 6:01 AM, Will Dietz <span dir="ltr"><<a href="mailto:wdietz2@illinois.edu" target="_blank">wdietz2@illinois.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Tue, Jan 8, 2013 at 3:02 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>

> On Tue, Jan 8, 2013 at 12:21 AM, Will Dietz <<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>> wrote:<br>
>> Thanks for taking a look.  Responses inline.<br>
>><br>
>> On Mon, Jan 7, 2013 at 10:16 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>> Thanks! This patch is highlighting a pressing need to refactor out the<br>
>>> repetition in ubsan_handlers.cc...<br>
>>><br>
>><br>
>> Heh, yep.  One day... :).<br>
>><br>
>>> The atomic compare-exchange isn't sufficient to remove the data race,<br>
>>> since the copy to the local Location object is non-atomic. How about<br>
>>> something like...<br>
>>><br>
>>> SourceLocation SourceLocation::acquire() {<br>
>>>   return SourceLocation(Filename, Line,<br>
>>> __sync_val_compare_and_swap(&Column, Column, ~u32(0)));<br>
><br>
> Oops, this is actually wrong, too, since the load of Column is<br>
> non-atomic. What we actually want here is<br>
><br>
>   __atomic_exchange_n(&Column, ~u32(0), __ATOMIC_RELAXED)<br>
><br>
> ... and this should be much quicker than __sync_* on some platforms,<br>
> since it's not a synchronization operation. The downside is that this<br>
> is pretty new (g++ 4.7 and Clang 3.1), so we should wrap it in #ifdef<br>
> __ATOMIC_RELAXED and use the __sync_ builtin otherwise.<br>
><br>
<br>
</div>Sounds good to me, and thank you for your explanation!  Certainly<br>
clears things up about the UB.<br>
<br>
Accordingly, I've removed all racing accesses to the Column field<br>
(including the misguided optimization) and<br>
switched to using __sync_lock_test_and_set when __atomic_exchange_n is<br>
unavailable.  This avoids the UB load and avoids the need for a full<br>
barrier (or a compare) as compared to the CAS approach.  TAS is<br>
guaranteed at least acquire semantics, which is more than we need but<br>
sufficient (Clang however gives it seq_cst, not sure why).<br>
<br>
Tricky business, thanks for helping me get this right! :)<br></blockquote><div><br></div><div style>Can you re-use atomic_exchange defined in sanitizer_common library? I took a glance at</div><div style>atomic_exchange in sanitizer_atomic_clang.h and it uses __sync_lock_test_and_set.</div>
<div style>Maybe, we should put optional use of __atomic_exchange_n there?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
>>> }<br>
>>> bool SourceLocation::isDisabled() {<br>
>>>   return Column == ~u32(0);<br>
>>> }<br>
>>><br>
>>> // ... then ...<br>
>>> SourceLocation Loc = Data->Loc.acquire();<br>
>>> if (Loc.isDisabled())<br>
>>>   return;<br>
>>><br>
>><br>
>> I like how this forces the copying of the SourceLocation in order to<br>
>> use the deduplication feature, something I didn't like about the<br>
>> approach I used.<br>
>><br>
>> That said, I'm not sure I understand your concern about the local<br>
>> SourceLocation copy.  Would you mind briefly explaining?  As far as I<br>
>> can tell, the load used for the copy must happen before our own CAS,<br>
>> and if the load in the local copy contains anything other than the<br>
>> original pristine Column value then we'll see the full ~u32(0) value<br>
>> in the CAS and discard the copy/loaded value without using it.  I<br>
>> suppose the question is if such a raced load is UB (unused or not,<br>
>> this is wrong) or simply an 'undef' value (like uninitialized,<br>
>> harmless if not used).  My reading says it's 'undef', but it's quite<br>
>> possible I'm missing something.  Thoughts?  Thanks!<br>
><br>
> In theory, C++11 [intro.multithread]p21: "The execution of a program<br>
> contains a data race if it contains two conflicting actions in<br>
> different threads, at least one of which is not atomic, and neither<br>
> happens before the other. Any such data race results in<br>
> undefined behavior."<br>
><br>
> In practice, since the compiler knows that a non-atomic load of Column<br>
> dominates the atomic exchange of Column, it also knows there can be no<br>
> concurrent modifications, so it can transform the atomic exchange into<br>
> an atomic store, which defeats your attempt to emit only one<br>
> diagnostic. I'm not sure whether there are compilers which actually do<br>
> that (yet).<br>
><br>
<br>
</div></div>Again, thanks for the detailed explanation.  Makes good sense, and<br>
much appreciated! :)<br>
<div class="im"><br>
>> Additionally, one intentional feature of the approach used in my patch<br>
>> is to avoid the call to __sync_bool_compare_and_swap() in the<br>
>> potentially 'common' case of the diagnostic indeed being disabled.<br>
>> This helps performance on programs with many concurrent duplicate<br>
>> overflows.  Does this seem valid to you?<br>
><br>
> No, for the same reason. But __atomic_exchange_n might be faster than<br>
> a branch anyway.<br>
><br>
<br>
</div>Sounds good to me.  This is likely plenty fast.<br>
<br>
Updated patches attached, thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Will<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
>>> On Mon, Jan 7, 2013 at 3:17 PM, Will Dietz <<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>> wrote:<br>
>>>> Updated, slightly neater patches attached.<br>
>>>><br>
>>>> Thanks!<br>
>>>><br>
>>>> ~Will<br>
>>>><br>
>>>> On Tue, Jan 1, 2013 at 9:49 PM, Will Dietz <<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>> wrote:<br>
>>>>> Updated to apply cleanly to latest clang/compiler-rt.<br>
>>>>><br>
>>>>> Thanks!<br>
>>>>><br>
>>>>> ~Will<br>
>>>>><br>
>>>>> On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz <<a href="mailto:wdietz2@illinois.edu">wdietz2@illinois.edu</a>> wrote:<br>
>>>>>> (Moving to cfe-commits@, was previously:<br>
>>>>>> <a href="http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html</a>)<br>
>>>>>><br>
>>>>>> Please see attached! :)<br>
>>>>>><br>
>>>>>> Thank you,<br>
>>>>>><br>
>>>>>> ~Will<br>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div>Alexey Samsonov, MSK</div>
</div></div>