Bug 1029245 - part 0 - tweak Skia's SkOnce.h header to work around issues with std::atomic::compare_exchange_strong; r=lsalzman
authorNathan Froyd <froydnj@mozilla.com>
Wed, 21 Dec 2016 04:28:08 -0500
changeset 452470 52741577a76d42e86d3ac351e36daae8175b78ac
parent 452469 73c80c1709a3ac2a8f085ce297d880cb92c6f7f5
child 452471 ac4575135a2a9a4cec556b9770b4878de6f4881f
push id39415
push usermozilla@noorenberghe.ca
push dateWed, 21 Dec 2016 20:49:14 +0000
reviewerslsalzman
bugs1029245
milestone53.0a1
Bug 1029245 - part 0 - tweak Skia's SkOnce.h header to work around issues with std::atomic::compare_exchange_strong; r=lsalzman Building Skia inside of mozilla-central with GCC 4.9.4 causes problems: [...]c++/4.9.4/bits/atomic_base.h:581:70: error: failure memory model cannot be stronger than success memory model for '__atomic_compare_exchange' The error stack accompanying this message points at SkEventTracer::GetInstance: SkEventTracer* SkEventTracer::GetInstance() { if (SkEventTracer* tracer = sk_atomic_load(&gUserTracer, sk_memory_order_acquire)) { return tracer; } static SkOnce once; static SkDefaultEventTracer* defaultTracer; once([] { defaultTracer = new SkDefaultEventTracer; }); return defaultTracer; } The only place that compare_exchange_strong could be called here is from SkOnce::operator(): template <typename Fn, typename... Args> void operator()(Fn&& fn, Args&&... args) { auto state = fState.load(std::memory_order_acquire); if (state == Done) { return; } // If it looks like no one has started calling fn(), try to claim that job. if (state == NotStarted && fState.compare_exchange_strong(state, Claimed, std::memory_order_relaxed)) { // Great! We'll run fn() then notify the other threads by releasing Done into fState. fn(std::forward<Args>(args)...); return fState.store(Done, std::memory_order_release); } [...code elided...] where |fState| is an atomic<uint8_t>. The three-argument form of atomic<uint8_t>::compare_exchange_strong is defined as: _GLIBCXX_ALWAYS_INLINE bool compare_exchange_strong(__int_type& __i1, __int_type __i2, memory_order __m = memory_order_seq_cst) noexcept { return compare_exchange_strong(__i1, __i2, __m, __cmpexch_failure_order(__m)); } __cmpexch_failure_order relaxes the given memory_order: // Drop release ordering as per [atomics.types.operations.req]/21 constexpr memory_order __cmpexch_failure_order2(memory_order __m) noexcept { return __m == memory_order_acq_rel ? memory_order_acquire : __m == memory_order_release ? memory_order_relaxed : __m; } constexpr memory_order __cmpexch_failure_order(memory_order __m) noexcept { return memory_order(__cmpexch_failure_order2(__m & __memory_order_mask) | (__m & __memory_order_modifier_mask)); } which then gets us to the four-argument version of compare_exchange_strong: _GLIBCXX_ALWAYS_INLINE bool compare_exchange_strong(__int_type& __i1, __int_type __i2, memory_order __m1, memory_order __m2) noexcept { memory_order __b2 = __m2 & __memory_order_mask; memory_order __b1 = __m1 & __memory_order_mask; __glibcxx_assert(__b2 != memory_order_release); __glibcxx_assert(__b2 != memory_order_acq_rel); __glibcxx_assert(__b2 <= __b1); return __atomic_compare_exchange_n(&_M_i, &__i1, __i2, 0, __m1, __m2); } Despite the constexpr annotation on __cmpexch_failure_order and friends, which ought to imply that they get constant-folded, I think what is happening is that GCC doesn't see |memory_order_relaxed| when it examines __m2. Instead, it seems some internal tree representation for the call to __cmpexch_failure_order. Since this is not an integer constant, GCC treats __m2 as being equivalent to memory_order_seq_cst (see gcc/builtins.c:get_memmodel). And since memory_order_seq_cst is stronger than memory_order_relaxed, we get the above error. In any event, the easiest fix is to simply use the four-argument form of compare_exchange_strong directly, explicitly specifying the failure memory order.
gfx/skia/skia/include/private/SkOnce.h
--- a/gfx/skia/skia/include/private/SkOnce.h
+++ b/gfx/skia/skia/include/private/SkOnce.h
@@ -26,16 +26,17 @@ public:
         auto state = fState.load(std::memory_order_acquire);
 
         if (state == Done) {
             return;
         }
 
         // If it looks like no one has started calling fn(), try to claim that job.
         if (state == NotStarted && fState.compare_exchange_strong(state, Claimed,
+                                                                  std::memory_order_relaxed,
                                                                   std::memory_order_relaxed)) {
             // Great!  We'll run fn() then notify the other threads by releasing Done into fState.
             fn(std::forward<Args>(args)...);
             return fState.store(Done, std::memory_order_release);
         }
 
         // Some other thread is calling fn().
         // We'll just spin here acquiring until it releases Done into fState.