False positive with asm release semantic and atomic_compare_and_exchange

[GoogleCodeExporter]

Full story described here https://github.com/OpenImageIO/oiio/issues/720.

Short story : 
https://github.com/OpenImageIO/oiio/blob/master/src/include/thread.h l.542 
implements a spin lock.
TSan reports data race with one write l.568
> // Fastest way to do it is with a store with "release" semantics
> __asm__ __volatile__("": : :"memory");
> m_locked = 0;  <<<< this line

and a compare and exchange l.597
> return atomic_compare_and_exchange (&m_locked, 0, 1);

m_locked is of type `volatile int`.

There is already a debate in the above mentioned thread about the usefulness of 
volatile. I'm not a low level concurrency expert so I actually don't know if 
the asm code has the correct semantic or not.

Environment:
- clang version 3.3 (tags/RELEASE_33/final)
- Arch Linux x86_64, kernel 3.11.6-1-ARCH

Original issue reported on code.google.com by chatelet...@gmail.com on 15 Nov 2013 at 9:20

[GoogleCodeExporter]

Hi,

Yes, volatile is not related to multi-threading in any way. So, yes, you have a 
data race and undefined behavior according to C/C++ standards. If you are using 
atomics-based fine-grained concurrent algorithms, you really need some good 
atomics API. When you have such an API, tsan will either understand it, or you 
can teach tsan to understand it.

tsan understands __sync builtin atomics, __atomic builtin atomics, __c11_atomic 
builtin atomics, std::atomic<> if it's implemented on top of builtin atomics.

If you use sufficiently new compilers, I would recommend using __atomic atomics.
If you need to support old compilers as well, then you need to do something 
along the lines:

int my_atomic_int_load_acquire(int *p) {
#ifdef OLD_COMPILER
  use your own implementation here
#else  // NEW_COMPILER
  return __atomic_load_n(p, ATOMIC_ACQUIRE);
#endif
}

this way tsan will automatically understand all your concurrent algorithms, 
because it's based on a new compiler.

Original comment by dvyu...@google.com on 15 Nov 2013 at 9:32

[GoogleCodeExporter]

Tsan is not going to acknowledge volatile as synchronization.
If they are spread over codebase, then this is a bug.
If they are used in atomics implementation, that the implementation must be 
somehow annotated for tsan.

Original comment by dvyu...@google.com on 26 Dec 2013 at 2:22

• Changed state: WontFix