ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

atomic ops using compiler intrinsics are not safe #76

Open bremoran opened 8 years ago

bremoran commented 8 years ago

armcc documentation says this:

10.139 strex intrinsic Note The compiler does not guarantee to preserve the state of the exclusive monitor. It might generate load and store instructions between the LDREX instruction generated for the ldrex intrinsic and the STREX instruction generated for the strex intrinsic. Because memory accesses can clear the exclusive monitor, code using the ldrex and __strex intrinsics can have unexpected behavior. Where LDREX and STREX instructions are needed, ARM recommends using embedded assembly.

@mjs-arm assures me that gcc could potentially reorder around our use of __LDREX and __STREX as well.

We should use gcc's atomic intrinsics for gcc targets. Of particular interest are:

cc @bogdanm @mjs-arm @rgrover

bogdanm commented 8 years ago

OK, but what about armcc?

ciarmcom commented 8 years ago

ARM Internal Ref: IOTSFW-2081

0xc0170 commented 8 years ago

I am aware of this for ARMCC: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0472l/chr1359125006834.html (v5.06, same is for older versions). I haven't tested if __atomic are available with cpp11 flag. I assume that because of this, the --gnu flag defines older GCC_VERSION so checking for it should work. I recall there was one problem reported on mbed when we updated the version (for reference: https://developer.mbed.org/questions/61245/Did-the-GCC-built-in-__atomic-functions-/)

This might require check version as I recall gcc >v4.7 include support for c++11 memory model.

bremoran commented 8 years ago

I believe that we should use __sync_* on armcc, but __atomic_* are recommended for new code on gcc.

bogdanm commented 8 years ago

We should also check support for this in other toolchains. Another option (that doesn't rely on compiler support) is to re-write these functions in assembler.

bremoran commented 8 years ago

@bogdanm Yes, that is the fallback position that I am trying to avoid. I'm trying to avoid it because every toolchain uses a different asm format, so we either need to generate asm from some intermediate format or we need to maintain N copies of the assembly functions for N compilers. Both of these are significantly more work than using the atomic intrinsics provided by the compiler.

Since each compiler seems to support at least some form of atomic intrinsic, we could add them to compiler-polyfill. The danger there is that using the atomic intrinsics directly may not generate correct disable/enable interrupt pairs on cortex-M0, and the presence of intrinsics in compiler-polyfill might encourage users to use them directly. I'm going to do some experiments with compiler intrinsics before we go any further.

bogdanm commented 8 years ago

Yes, that is the fallback position that I am trying to avoid. I'm trying to avoid it because every toolchain uses a different asm format, so we either need to generate asm from some intermediate format or we need to maintain N copies of the assembly functions for N compilers. Both of these are significantly more work than using the atomic intrinsics provided by the compiler.

Absolutely, which is why I've been wondering how difficult (or weird) would be to write all our ASM functions using only GCC ASM, but use them with all toolchains. Sure, it looks insanely strange if you need two different toolchains to compile mbed OS :), but maybe there are better ways to do this (I didn't find a sane one yet).

ghost commented 8 years ago

It appears that armcc5, llvm (and hence armcc6) all claim to support GCCs sync_ primitives. Therefore it should be possible for this set of compilers to write these primitives using _sync* intrinsics. I'm not sure what degree of support there is for the more recent (and much more flexible) atomic_* intrinsics provided by GCC.

Sync primitives are documented here: https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/_005f_005fsync-Builtins.html#_005f_005fsync-Builtins

Atomic primitives are documented here: https://gcc.gnu.org/onlinedocs/gcc-5.3.0/gcc/_005f_005fatomic-Builtins.html#_005f_005fatomic-Builtins

The atomic primitives were added in order to better support the c++11 memory model, there is a link on the page above to a document discussing how the atomic primitives map to c++11 memory model.