Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing __atomic_load & __atomic_store intrinsics' implementation for esp32c3 (& maybe esp32s2) (IDFGH-5896) #7591

Closed
ivmarkov opened this issue Sep 23, 2021 · 3 comments
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@ivmarkov
Copy link
Contributor

The implementation of atomics here is broken for Rust+LLVM+ESP-IDF+esp32c3 in that it does not define the __atomic_load and __atomic_store intrinsics for esp32c3. By the way, it does not define these for esp32s2 either?

It might be a simple omission, or it might be a misinterpretation of what the compiler (Rust + LLVM at least, not sure for GCC) would generate in terms of atomic libcalls on the Riscv32IMC architecture:

  • The compiler WILL generate calls to __atomic_load / __atomic_store even though loads/stores on that architecture are atomic, because - as per this comment - it is not safe to mix CPU-implemented atomicity (load/store) with libcall-implemented atomicity (e.g. CAS). Since CAS is not natively supported on Riscv32IMC, Rust+LLVM in fact does generate calls to __atomic_load / __atomic_store.

Suggested fix: just add

ATOMIC_LOAD(1, uint8_t)
ATOMIC_LOAD(2, uint16_t)
ATOMIC_LOAD(4, uint32_t)

ATOMIC_STORE(1, uint8_t)
ATOMIC_STORE(2, uint16_t)
ATOMIC_STORE(4, uint32_t)

... here.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Sep 23, 2021
@github-actions github-actions bot changed the title Missing __atomic_load & __atomic_store intrinsics' implementation for esp32c3 (& maybe esp32s2) Missing __atomic_load & __atomic_store intrinsics' implementation for esp32c3 (& maybe esp32s2) (IDFGH-5896) Sep 23, 2021
@ivmarkov
Copy link
Contributor Author

@igrr ^^^

(Playing with ESP-IDF master in cargo-native build and - to my surprise - got undefined reference to __atomic_load/store_NN from the linker.)

Implementing the suggested fix takes care of the linker errors.

@MabezDev
Copy link
Collaborator

Ah yes, so this is because GCC implemented the native load/stores (that are theoretically UB, whether it happens in practice is unknown) but as you know from that link, its not implemented in llvm. I think your fix is correct, and simple to implement.

@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Resolution: NA Issue resolution is unavailable labels Oct 12, 2021
@igrr igrr closed this as completed in d93b53b Oct 12, 2021
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Oct 12, 2021

@igrr The committed fix is not quite correct, because the missing atomics are ONLY implemented when building with clang

However, we need them when the ESP-IDF is built with GCC too, because:

  • While the Rust code is (always, for now) built with LLVM
  • ... the ESP-IDF can be built with GCC, and even in that build mode, it does have to provide the above atomics, for Rust+LLVM, who is always lowering the load/store (on RiscV) and test-lock-and-set (for esp32s2) to libcalls...

espressif-bot pushed a commit that referenced this issue Dec 24, 2021
Provide emulated atomic load & store libcalls for u8, u16 & u32 integer
types. This is required when building with Clang as llvm does not lower
these operations to native load / stores, where as gcc does.

Provide `sync_lock_test_and_set` atomic implementations for all
supported integer types.

Closes #7591.
Closes #7592.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

3 participants