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

oreboot won't build for riscv64: __atomic_{load,store}_16 are not defined #66240

Closed
rminnich opened this issue Nov 9, 2019 · 10 comments
Closed
Labels
C-bug Category: This is a bug. O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems

Comments

@rminnich
Copy link

rminnich commented Nov 9, 2019

We have been building github.com/oreboot/oreboot with rust since last spring. Within the last month, we've hit the problem seen in https://dev.azure.com/azure0427/Oreboot%20Pipeline/_build/results?buildId=338, as well as oreboot/oreboot#183

__atomic_store_16 is undefined, as is __atomic_load_16.

oreboot has been working on riscv hardware (riscv64, sifive hifive board) for several months, so we're not quite sure where to start. I've been looking around for similar problems but nothing quite fits.

Thanks.

@jonas-schievink jonas-schievink added O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems C-bug Category: This is a bug. labels Nov 9, 2019
@mati865
Copy link
Contributor

mati865 commented Nov 9, 2019

Already reported to upstream crate: rust-lang/compiler-builtins#322

@rminnich
Copy link
Author

here is the "fix" for oreboot

`+.globl __atomic_store_16
+__atomic_store_16:

  •   ret
    

+.globl __atomic_load_16
+__atomic_load_16:

  •   ret
    

`

i.e. fake up no op atomic load and store for 16 bits, b/c the architecture does not support them.

You're asking the architecture to do something it can't do ... :-p

@rminnich
Copy link
Author

sorry about the format, that's what github claims is a code copy/paste?
`
+.globl __atomic_store_16
+__atomic_store_16:

  •   ret
    

+.globl __atomic_load_16
+__atomic_load_16:

  •   ret
    

.globl _stack_ptr
.section .rodata
_stack_ptr: .word 0x08010000
`

@asb
Copy link

asb commented Nov 12, 2019

Copying comment from oreboot/oreboot#183 (comment)
CC @lenary
IIRC In the GCC world this symbol is provided by libatomic rather than libgcc directly. In the LLVM world, compiler-rt provides the symbol. Rust should either use a RISC-V compiler-rt or if using libgcc it should link libatomic as well.

@lenary
Copy link
Contributor

lenary commented Nov 12, 2019

Yeah. This issue comes up every so often, especially more recently.

The RISC-V "A" extension only introduces instructions for 32- and 64-bit atomic operations. If you need up-to-32-bit atomic operations (ie, 8-bit or 16-bit atomic operations), then you have to link with -latomic if using gcc's compiler builtins (because libgcc and libatomic are separate libraries).

LLVM's compiler-rt is a slightly different, and contains both the compiler builtins and the atomic libcalls in the same library, so this issue should arise less once LLVM can switch to compiler-rt for RISC-V. That could happen soon given https://reviews.llvm.org/D68393 has landed.

@mati865
Copy link
Contributor

mati865 commented Nov 13, 2019

I was fixed in compiler-builtins crate via rust-lang/compiler-builtins#324.
Can you make xargo pull newer version? Otherwise it may require updating this dependency in Rust repo.

bors pushed a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2023
This roughly reverts PR rust-lang#66548

Atomic "CAS" are still disabled for targets without the
*“A” Standard Extension for Atomic Instructions*.
However this extension only adds instructions for operations more complex
than simple loads and stores, which are always atomic when aligned.

In the [Unprivileged Spec v. 20191213](https://riscv.org/technical/specifications/)
section 2.6 *Load and Store Instructions* of
chapter 2 *RV32I Base Integer Instruction Set* (emphasis mine):

> Even when misaligned loads and stores complete successfully,
> these accesses might run extremely slowly depending on the implementation
> (e.g., when implemented via an invisible trap). Further-more, whereas
> **naturally aligned loads and stores are guaranteed to execute atomically**,
> misaligned loads and stores might not, and hence require
> additional synchronization to ensure atomicity.

Unfortunately PR rust-lang#66548 did not provide
much details on the bug that motivated it, but
rust-lang#66240 and
rust-lang#85736 appear related
and happen with targets that do have the A extension.
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 5, 2023
Re-enable atomic loads and stores for all RISC-V targets

This roughly reverts PR rust-lang#66548

Atomic "CAS" are still disabled for targets without the *“A” Standard Extension for Atomic Instructions*. However this extension only adds instructions for operations more complex than simple loads and stores, which are always atomic when aligned.

In the [Unprivileged Spec v. 20191213](https://riscv.org/technical/specifications/) section 2.6 *Load and Store Instructions* of chapter 2 *RV32I Base Integer Instruction Set* (emphasis mine):

> Even when misaligned loads and stores complete successfully, these accesses might run extremely slowly depending on the implementation (e.g., when implemented via an invisible trap). Further-more, whereas **naturally aligned loads and stores are guaranteed to execute atomically**, misaligned loads and stores might not, and hence require additional synchronization to ensure atomicity.

Unfortunately PR rust-lang#66548 did not provide much details on the bug that motivated it, but rust-lang#66240 and rust-lang#85736 appear related and happen with targets that do have the A extension.
@istankovic
Copy link
Contributor

@rminnich @lenary is this issue now fixed?

@workingjubilee
Copy link
Member

cc @kito-cheng @michaelmaitland @robin-randhawa-sifive Is this issue still an active concern?

@taiki-e
Copy link
Member

taiki-e commented Oct 6, 2024

First, the suffix of the GCC atomic builtins is in bytes, not in bits, so __atomic_*_16 is 16-byte (128-bit), not 16-bit.

And, there is a relatively recently ratified Zacas extension supports 128-bit atomic in RISC-V, but it did not exist then and in any case LLVM generates libcalls when 128-bit atomic intrinsics are used on RISC-V.

So, I believe this has been fixed by rust-lang/compiler-builtins#324, which fixed the problem of calling 128-bit atomic intrinsics on unsupported platforms. (I began writing RISC-V-related code later than that fix, but at least I have never seen "undefined symbol" error about atomics when compiling binaries for RISC-V bare-metal/Linux, except for a regression I fixed in #114497.)

(Btw, AFAIK there are no ratified 16-bit atomic instructions (AMO,LR/SC) in RISC-V except those related to the recently ratified Zabha extension, but 16-bit load/store are exist, and it is possible to implement 16-bit CAS/RMW using 32-bit atomic instructions, and LLVM actually does so.)

@workingjubilee
Copy link
Member

Alright, I'm assuming this has been fixed then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-riscv Target: RISC-V architecture T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems
Projects
None yet
Development

No branches or pull requests

8 participants