Clang generates atomic memory ordering warnings when compiling atomicops_internals_generic_gcc.h

[edmonds]

Hi,

When protobuf is patched to expose the generic atomicops implementation on Clang as in PR #21, the following warnings are generated:

./google/protobuf/stubs/atomicops_internals_generic_gcc.h:86:32: warning: memory order argument to atomic operation is invalid [-Watomic-memory-ordering]
  __atomic_store_n(ptr, value, __ATOMIC_ACQUIRE);
                               ^~~~~~~~~~~~~~~~

./google/protobuf/stubs/atomicops_internals_generic_gcc.h:102:31: warning: memory order argument to atomic operation is invalid [-Watomic-memory-ordering]
  return __atomic_load_n(ptr, __ATOMIC_RELEASE);
                              ^~~~~~~~~~~~~~~~

According to the gcc documentation at https://gcc.gnu.org/onlinedocs/gcc-4.9.1/gcc/_005f_005fatomic-Builtins.html, these arguments are indeed invalid. __atomic_store_n() cannot be passed __ATOMIC_ACQUIRE and __atomic_load_n() cannot be passed __ATOMIC_RELEASE.

I asked about this on IRC and got the following response from someone knowledgeable about atomic memory ordering.

"release + load (what the function name indicates) usually isn't a very interesting combination of behaviours."

"I doubt it has many users in protobuf?"

"if you want to get rid of the warnings you can probably add a explicit __atomic_thread_fence(__ATOMIC_RELEASE) and use __ATOMIC_RELAXED in the actually intended atomic."

"Alternatively just promote them to SEQ_CST ;)"

Indeed, a git grep of the protobuf repository shows that there aren't actually any users of the Acquire_Store() and Release_Load() functions.

[xfxyjwf]

Could you help make a patch and remove these two methods? I don't think we are using them in protobuf.

[edmonds]

Sure, that's easier than any of the other options :-)

[edmonds]

Actually, I just looked at the TSAN atomics implementation, and it uses a full SEQ_CST fence, which is the strongest memory model.

https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/atomicops_internals_tsan.h#L109

https://github.com/google/protobuf/blob/master/src/google/protobuf/stubs/atomicops_internals_tsan.h#L125

Changing the generic atomics to match that would be trivial.