Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

atomic ops using compiler intrinsics are not safe #76

Open
bremoran opened this issue Feb 18, 2016 · 8 comments
Open

atomic ops using compiler intrinsics are not safe #76

bremoran opened this issue Feb 18, 2016 · 8 comments
Labels

Comments

@bremoran
Copy link
Contributor

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:

  • __atomic_compare_exchange
  • __atomic_add_fetch
  • __atomic_sub_fetch

cc @bogdanm @mjs-arm @rgrover

@bogdanm
Copy link
Contributor

bogdanm commented Feb 18, 2016

OK, but what about armcc?

@ciarmcom
Copy link
Member

ARM Internal Ref: IOTSFW-2081

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 19, 2016

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
Copy link
Contributor Author

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

@bogdanm
Copy link
Contributor

bogdanm commented Feb 19, 2016

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
Copy link
Contributor Author

@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
Copy link
Contributor

bogdanm commented Feb 19, 2016

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
Copy link

ghost commented Feb 19, 2016

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants