-
Notifications
You must be signed in to change notification settings - Fork 15.9k
Description
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.