libsanitizer mege from upstream r169371

Dmitry Vyukov dvyukov@google.com
Wed Dec 5 12:53:00 GMT 2012


On Wed, Dec 5, 2012 at 4:42 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 05, 2012 at 03:13:20PM +0400, Dmitry Vyukov wrote:
>> I hope I addressed all your comments in this revision.
>> I've fixed nand atomic operation, made atomic operations atomic again
>> and added visibility attribute to interface functions.
>
> Mostly, still, __sync_lock_test_and_set isn't full barrier unlike most other
> __sync_* builtins, so either you'd need to use __atomic_exchange_n if
> available (you can e.g. guard it with #ifdef __ATOMIC_SEQ_CST), or
> if not available also use __sync_synchronize ().


__atomic_xxx are available since gcc 4.7 and we use gcc 4.4 on some
bots. I do not want to clutter the code with macros too much.
I will add:

 template<typename T> T func_xchg(volatile T *v, T op) {
-  return __sync_lock_test_and_set(v, op);
+  T res = __sync_lock_test_and_set(v, op);
+  // __sync_lock_test_and_set does not contain full barrier.
+  __sync_synchronize();
+  return res;
 }


> Also, for the atomic load and store, it would be nice to use __atomic_load_n
> and __atomic_store_n if available (as fallback __sync_synchronize (), which
> I believe needs to be before resp. after the memory operation depending on
> whether it is load or store).

It would be nice to have atomic operations in C/C++ from day one, it
would make our lives so much easier :)
My current code contains some additional memory barrier:

template<typename T>
static T AtomicLoad(ThreadState *thr, uptr pc, const volatile T *a,
    morder mo) {
...
  T v = *a;
...
  __sync_synchronize();
...
}

template<typename T>
static void AtomicStore(ThreadState *thr, uptr pc, volatile T *a, T v,
...
  __sync_synchronize();
...
  *a = v;
...
}

OK, I see what you mean. There is missing trailing barrier. I will add:

   thr->clock.ReleaseStore(&s->clock);
   *a = v;
   s->mtx.Unlock();
+  // Trainling memory barrier to provide sequential consistency
+  // for Dekker-like store-load synchronization.
+  __sync_synchronize();
 }

Actually on x86 (the only platform that we support now), the mutex
provides full barriers on both lock and unlock (because it's based on
atomic RMW) :)




> And we'll need to tweak libsanitizer makefiles, so it uses -mcx16 on x86_64
> 64-bit when compiling that tsan_atomic*.cc, either we just ignore for
> libsanitizer the pre-CX16 AMD CPUs, those were just a few initial models
> I think, or we'd need to do IFUNC dispatcher, or perhaps just one time
> cpuid check that would see if it can do 16-byte atomics or not.

I think we just add -mcx16.

Thanks!



More information about the Gcc-patches mailing list