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

[Will Dietz]

On Mon, Jan 14, 2013 at 1:27 AM, Alexey Samsonov <samsonov at google.com> wrote:
>
>
>
> On Wed, Jan 9, 2013 at 6:01 AM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>
>> On Tue, Jan 8, 2013 at 3:02 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Tue, Jan 8, 2013 at 12:21 AM, Will Dietz <wdietz2 at illinois.edu>
>> > wrote:
>> >> 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)));
>> >
>> > Oops, this is actually wrong, too, since the load of Column is
>> > non-atomic. What we actually want here is
>> >
>> >   __atomic_exchange_n(&Column, ~u32(0), __ATOMIC_RELAXED)
>> >
>> > ... and this should be much quicker than __sync_* on some platforms,
>> > since it's not a synchronization operation. The downside is that this
>> > is pretty new (g++ 4.7 and Clang 3.1), so we should wrap it in #ifdef
>> > __ATOMIC_RELAXED and use the __sync_ builtin otherwise.
>> >
>>
>> Sounds good to me, and thank you for your explanation!  Certainly
>> clears things up about the UB.
>>
>> Accordingly, I've removed all racing accesses to the Column field
>> (including the misguided optimization) and
>> switched to using __sync_lock_test_and_set when __atomic_exchange_n is
>> unavailable.  This avoids the UB load and avoids the need for a full
>> barrier (or a compare) as compared to the CAS approach.  TAS is
>> guaranteed at least acquire semantics, which is more than we need but
>> sufficient (Clang however gives it seq_cst, not sure why).
>>
>> Tricky business, thanks for helping me get this right! :)
>
>
> Can you re-use atomic_exchange defined in sanitizer_common library? I took a
> glance at
> atomic_exchange in sanitizer_atomic_clang.h and it uses
> __sync_lock_test_and_set.
> Maybe, we should put optional use of __atomic_exchange_n there?
>

Was looking at this, went ahead and moved to shared impl in r172429, thanks.

Looks like using __atomic_exchange_n was already discussed and its use
postponed[1].

As I discovered with a previous revision, checking for __ATOMIC_* to
be defined is not sufficient on some platforms[2].

Not sure how to best test for support, some mix of
__GXX_EXPERIMENTAL_CXX0X__ and
__has_extension(cxx_atomic)/has_feature(cxx_atomic) seems like the
best we can do.

Thoughts?  Perhaps not worth it?

~Will

[1] http://comments.gmane.org/gmane.comp.gcc.patches/277763
[2] http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130107/161480.html

>>
>>
>> >>> }
>> >>> 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!
>> >
>> > In theory, C++11 [intro.multithread]p21: "The execution of a program
>> > contains a data race if it contains two conflicting actions in
>> > different threads, at least one of which is not atomic, and neither
>> > happens before the other. Any such data race results in
>> > undefined behavior."
>> >
>> > In practice, since the compiler knows that a non-atomic load of Column
>> > dominates the atomic exchange of Column, it also knows there can be no
>> > concurrent modifications, so it can transform the atomic exchange into
>> > an atomic store, which defeats your attempt to emit only one
>> > diagnostic. I'm not sure whether there are compilers which actually do
>> > that (yet).
>> >
>>
>> Again, thanks for the detailed explanation.  Makes good sense, and
>> much appreciated! :)
>>
>> >> 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?
>> >
>> > No, for the same reason. But __atomic_exchange_n might be faster than
>> > a branch anyway.
>> >
>>
>> Sounds good to me.  This is likely plenty fast.
>>
>> Updated patches attached, thanks!
>>
>> ~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
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
>
> --
> Alexey Samsonov, MSK