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

Use of RiscV "+forced-atomics" target feature triggers hard-float ABI for symbols.o #114153

Closed
paulmenage opened this issue Jul 28, 2023 · 3 comments · Fixed by #114332
Closed
Assignees
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. 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.

Comments

@paulmenage
Copy link
Contributor

paulmenage commented Jul 28, 2023

When building for RiscV-32, adding the "+forced-atomics" target feature (in this case, adding it to a custom target .json file) causes the symbols.o file to be built with the single-float ABI rather than the soft-float ABI used for the all .rcgu.o files, which causes the link to fail due to incompatible float ABIs.

It appears to be due to this code in compiler/rustc_codegen_ssa/src/back/metadata.rs:

        Architecture::Riscv32 | Architecture::Riscv64 => {
            // Source: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/079772828bd10933d34121117a222b4cc0ee2200/riscv-elf.adoc
            let mut e_flags: u32 = 0x0;
            let features = &sess.target.options.features;
            // Check if compressed is enabled
            if features.contains("+c") {
                e_flags |= elf::EF_RISCV_RVC;
            }

            // Select the appropriate floating-point ABI
            if features.contains("+d") {
                e_flags |= elf::EF_RISCV_FLOAT_ABI_DOUBLE;
            } else if features.contains("+f") {
                e_flags |= elf::EF_RISCV_FLOAT_ABI_SINGLE;
            } else {
                e_flags |= elf::EF_RISCV_FLOAT_ABI_SOFT;
            }
            e_flags
        }

which assumes that any feature beginning with "+f" is in fact exactly "+f", so the addition of "+forced-atomics" triggers the single-float ABI.

Should this check (and most other feature checks) actually be something like: features.split(",").any(|f| f == "+f") (probably useful as a TargetOptions::has_feature() helper method)?

@paulmenage paulmenage added the C-bug Category: This is a bug. label Jul 28, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 28, 2023
@bjorn3 bjorn3 added O-riscv Target: RISC-V architecture A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. labels Jul 28, 2023
@paulmenage
Copy link
Contributor Author

Cc: @nikic

@paulmenage paulmenage changed the title Use of RiscV "+forced-atomics" target feature triggers hard-float ABI Use of RiscV "+forced-atomics" target feature triggers hard-float ABI for symbols.o Jul 28, 2023
@Noratrieb
Copy link
Member

cc @nbdd0121 , I think you added this. I looked a bit through uses of target features and found

if let Some(feat) = [sess.opts.cg.target_feature.as_str(), &sess.target.options.features]
.into_iter()
.find(|feat| !feat.is_empty())
{
cmd.arg("--cpu-features");
cmd.arg(feat);
}

which looks pretty suspicious too. Fixing this bug is indeed simple but making sure this doesn't happen again is less so (although these two things of course don't have to happen in the same PR).

@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 29, 2023
@nbdd0121
Copy link
Contributor

I didn't recall changing this bit. This function has existed for quite a while for creating metadata files, I just also call it to create symbols.o.

That said, I'm happy to work on a fix.

@rustbot claim

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 8, 2023
…nieu

Revert rust-lang#98333 "Re-enable atomic loads and stores for all RISC-V targets"

This reverts rust-lang#98333.

As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

However, `forced-atomics` target feature is currently broken (rust-lang#114153), so AFAIK, there is currently no way to enable atomic load/store (via core::intrinsics) on these targets properly.

r? `@Amanieu`
@bors bors closed this as completed in 484cb4e Aug 20, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 29, 2023
Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf

As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

~~This PR is currently marked as a draft because:~~
- ~~`forced-atomics` target feature is currently broken (rust-lang#114153 EDIT: Fixed
- ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

r? `@Amanieu`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 29, 2023
Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf

As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

~~This PR is currently marked as a draft because:~~
- ~~`forced-atomics` target feature is currently broken (rust-lang#114153 EDIT: Fixed
- ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

r? `@Amanieu`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 30, 2023
Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf

As said in rust-lang#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

~~This PR is currently marked as a draft because:~~
- ~~`forced-atomics` target feature is currently broken (rust-lang#114153 EDIT: Fixed
- ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

r? `@Amanieu`
bors added a commit to rust-lang/miri that referenced this issue Nov 30, 2023
Pass +forced-atomics feature for riscv32{i,im,imc}-unknown-none-elf

As said in rust-lang/rust#98333 (comment), `forced-atomics` target feature is also needed to enable atomic load/store on these targets (otherwise, libcalls are generated): https://godbolt.org/z/433qeG7vd

~~This PR is currently marked as a draft because:~~
- ~~`forced-atomics` target feature is currently broken (rust-lang/rust#114153 EDIT: Fixed
- ~~`forced-atomics` target feature has been added in LLVM 16 (llvm/llvm-project@f5ed0cb), but the current minimum LLVM version [is 15](https://github.com/rust-lang/rust/blob/90f0b24ad3e7fc0dc0e419c9da30d74629cd5736/src/bootstrap/llvm.rs#L557). In LLVM 15, the atomic load/store of these targets generates libcalls anyway.~~ EDIT: LLVM 15 has been dropped

Depending on the policy on the minimum LLVM version for these targets, this may be blocked until the minimum LLVM version is increased to 16.

r? `@Amanieu`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants