[cfe-commits] [Review] [ubsan] Deduplication

[Will Dietz]

Thanks for taking a look.  Responses inline.

On Mon, Jan 7, 2013 at 10:16 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Thanks! This patch is highlighting a pressing need to refactor out the
> repetition in ubsan_handlers.cc...
>

Heh, yep.  One day... :).

> The atomic compare-exchange isn't sufficient to remove the data race,
> since the copy to the local Location object is non-atomic. How about
> something like...
>
> SourceLocation SourceLocation::acquire() {
>   return SourceLocation(Filename, Line,
> __sync_val_compare_and_swap(&Column, Column, ~u32(0)));
> }
> bool SourceLocation::isDisabled() {
>   return Column == ~u32(0);
> }
>
> // ... then ...
> SourceLocation Loc = Data->Loc.acquire();
> if (Loc.isDisabled())
>   return;
>

I like how this forces the copying of the SourceLocation in order to
use the deduplication feature, something I didn't like about the
approach I used.

That said, I'm not sure I understand your concern about the local
SourceLocation copy.  Would you mind briefly explaining?  As far as I
can tell, the load used for the copy must happen before our own CAS,
and if the load in the local copy contains anything other than the
original pristine Column value then we'll see the full ~u32(0) value
in the CAS and discard the copy/loaded value without using it.  I
suppose the question is if such a raced load is UB (unused or not,
this is wrong) or simply an 'undef' value (like uninitialized,
harmless if not used).  My reading says it's 'undef', but it's quite
possible I'm missing something.  Thoughts?  Thanks!

Additionally, one intentional feature of the approach used in my patch
is to avoid the call to __sync_bool_compare_and_swap() in the
potentially 'common' case of the diagnostic indeed being disabled.
This helps performance on programs with many concurrent duplicate
overflows.  Does this seem valid to you?  It's the primary reason for
having any of the non-atomic accesses to the Columns field in this
code.

Thank you for your time,

~Will

> On Mon, Jan 7, 2013 at 3:17 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>> Updated, slightly neater patches attached.
>>
>> Thanks!
>>
>> ~Will
>>
>> On Tue, Jan 1, 2013 at 9:49 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>> Updated to apply cleanly to latest clang/compiler-rt.
>>>
>>> Thanks!
>>>
>>> ~Will
>>>
>>> On Sun, Dec 30, 2012 at 6:22 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>>> (Moving to cfe-commits@, was previously:
>>>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-December/026519.html)
>>>>
>>>> Please see attached! :)
>>>>
>>>> Thank you,
>>>>
>>>> ~Will