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.
This commit is contained in:
Nathan Froyd 2016-12-21 04:28:08 -05:00
Родитель 35cc78d738
Коммит 0c4b96a65e
1 изменённых файлов: 1 добавлений и 0 удалений

Просмотреть файл

@ -31,6 +31,7 @@ public:
// 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)...);