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

Xous build failure due to naked_asm changes #134403

Closed
xobs opened this issue Dec 17, 2024 · 11 comments · Fixed by #134789
Closed

Xous build failure due to naked_asm changes #134403

xobs opened this issue Dec 17, 2024 · 11 comments · Fixed by #134789
Labels
A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS C-discussion Category: Discussion or questions that doesn't represent real issues. F-naked_functions `#![feature(naked_functions)]` O-xous OS: A microkernel OS for privacy in computing T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@xobs
Copy link
Contributor

xobs commented Dec 17, 2024

Code

I tried this code:

$ RUST_COMPILER_RT_ROOT="$(pwd)/src/llvm-project/compiler-rt" RUSTFLAGS="-Zforce-unstable-if-unmarked=yes" cargo +nightly build --target riscv32imac-unknown-xous-elf -Zbinary-dep-depinfo --release --features "panic-unwind" --manifest-path "library/sysroot/Cargo.toml"

I expected to see this happen: Rust should build libstd for Xous

Instead, this happened: Rust errored out with could not compile 'unwinding':

   Compiling unwinding v0.2.4
   Compiling unwind v0.0.0 (/opt/Xous/rust-nightly/rust/library/unwind)
   Compiling panic_unwind v0.0.0 (/opt/Xous/rust-nightly/rust/library/panic_unwind)
error: <inline asm>:11:1: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_def_cfa_offset 0x90
^

error: could not compile `unwinding` (lib)

Version it worked on

It most recently worked on: Rust nightly 2024-12-11

Version with regression

rustc --version --verbose:

rustc 1.85.0-nightly (6d9f6ae36 2024-12-16)
binary: rustc
commit-hash: 6d9f6ae36ae1299d6126ba40c15191f7aa3b79d8
commit-date: 2024-12-16
host: x86_64-unknown-linux-gnu
release: 1.85.0-nightly
LLVM version: 19.1.5

More information

I have filed an issue in unwinding. This error likely appeared as a result of #128004. However, I am unable to merge PR 41 because CI is failing due to issue #80608.

@rustbot modify labels: +regression-from-stable-to-nightly -regression-untriaged

@xobs xobs added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Dec 17, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Dec 17, 2024
@xobs
Copy link
Contributor Author

xobs commented Dec 17, 2024

I've created this Issue because Xous is now unable to build. There are several factors at play:

@workingjubilee
Copy link
Member

workingjubilee commented Dec 17, 2024

unwinding depends on #[cfg(target_feature)] which doesn't work in --release mode on RISC-V

hm. that should not be the case, to my knowledge? can you elaborate?

@xobs
Copy link
Contributor Author

xobs commented Dec 17, 2024

Sure.

#![feature(naked_functions)]

#[naked]
pub extern "C-unwind" fn example() {
    #[cfg(target_feature = "d")]
    unsafe {
        core::arch::naked_asm!("fsd fs0, 0x140(sp)");
    }
}

If you build this with cargo +nightly build --target riscv64gc-unknown-linux-gnu --release it fails with the following error:

error: <inline asm>:6:1: instruction requires the following: 'D' (Double-Precision Floating-Point)
fsd fs0, 0x140(sp)
^

Even though it's gated by the d flag that should only be active when the d target feature is present.

@Noratrieb Noratrieb added O-xous OS: A microkernel OS for privacy in computing regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 17, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 17, 2024
@Noratrieb
Copy link
Member

Amanieu said on the PR that this is intended behavior, so requiring CFI directives iss not an accidental regression. It will need to be fixed in unwinding.

@Noratrieb Noratrieb removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 17, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 17, 2024

Reproducing @Amanieu's comments on the PR here:

These directives need to be provided by the assembly author. It is up to them to declare that the naked function has valid unwind info.

[ ... ]

The previous behavior was actually context-dependent: the compiler may or may not insert cfi_startproc depending on various factors such as whether the panic mode is unwind or abort, whether unwind info is force-enabled, etc.

This makes writing code that uses CFI directives quite brittle and it's a problem I've hit before in libfringe (the predecessor of corosensei). It's much better to let users provide .cfi_startproc when they are providing unwind info for a naked function.

Unmarking this as a regression for the time being.
@rustbot label -regression-from-stable-to-nightly

@jieyouxu jieyouxu added C-discussion Category: Discussion or questions that doesn't represent real issues. F-asm `#![feature(asm)]` (not `llvm_asm`) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS F-naked_functions `#![feature(naked_functions)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed C-bug Category: This is a bug. F-asm `#![feature(asm)]` (not `llvm_asm`) labels Dec 17, 2024
@rustbot rustbot removed the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Dec 17, 2024
@workingjubilee
Copy link
Member

If you build this with cargo +nightly build --target riscv64gc-unknown-linux-gnu --release it fails with the following error:

@xobs Hm. So, the riscv64gc target should have the "d" feature enabled, because "g" includes "d", thus the assembly should be instantiated, and is. The problem isn't with the cfg(target_feature)... that's purely handled in Rust syntax, and the error you are getting is from LLVM. It is something more subtle which causes us to not communicate to LLVM the function context has the feature.

@xobs
Copy link
Contributor Author

xobs commented Dec 17, 2024

I marked it as a regression because it worked with nightly-2024-12-12 but fails with nightly-2024-12-13:

cat > test.rs <<EOF
#![feature(naked_functions)]

#[naked]
pub extern "C-unwind" fn example() {
    #[cfg(target_feature = "d")]
    unsafe {
        core::arch::naked_asm!("fsd fs0, 0x140(sp)");
    }
}
EOF

echo -n "Building with 2024-12-12: "
rustc +nightly-2024-12-12 --target riscv64gc-unknown-linux-gnu --crate-type lib -O test.rs
echo "Done"
echo -n "Building with 2024-12-13: "
rustc +nightly-2024-12-13 --target riscv64gc-unknown-linux-gnu --crate-type lib -O test.rs

This produces:

Building with 2024-12-12: Done
Building with 2024-12-13: error: <inline asm>:6:1: instruction requires the following: 'D' (Double-Precision Floating-Point)
fsd fs0, 0x140(sp)
^

What are my options to fix the build for this target? I cannot build unwinding which is a prerequisite, due to #80608.

@Amanieu
Copy link
Member

Amanieu commented Dec 17, 2024

You can work around it by wrapping the assembly code with:

".option push",
".option arch, +d",
// your asm here
".option push",

@xobs
Copy link
Contributor Author

xobs commented Dec 17, 2024

Thank you for that suggestion. That does work, and I'll submit a patch to the unwinding project to add that. It still seems like a regression to me, in the sense that it used to work but now doesn't.

I'll keep this issue open at least until Xous builds again. Once unwinding is building again I can submit a PR to libstd to bump the version number to fix the build here.

@Noratrieb
Copy link
Member

The reason it's not labelled as a regression is because we only track regressions for stable features or accidental nightly regressions. Nightly features may break, and in this case a nightly feature had expected breakage.

@workingjubilee
Copy link
Member

@xobs Marking it as a non-regression is not "this is something we don't care about"!

It's "our regression tracking tags are implicitly for the purpose of tracking things we want to actively prevent hitting the next stable/beta release". This results in the oddity of "we would like your target to be fixed as soon as possible but we aren't too worried if the next stable release can't build it because as far as we're concerned only specific nightly releases can".

jhpratt added a commit to jhpratt/rust that referenced this issue Dec 27, 2024
…5.0, r=Mark-Simulacrum

unwinding: bump version to fix naked_asm on Xous

With rust-lang#80608 the `unwinding` crate no longer builds. The upstream crate has been updated to build by manually adding directives to the naked_asm stream.

Bump the dependency in Rust to get this newer version. This fixes the build for Xous, and closes rust-lang#134403.
@bors bors closed this as completed in f806357 Dec 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 27, 2024
Rollup merge of rust-lang#134789 - betrusted-io:bump-unwinding-to-0.25.0, r=Mark-Simulacrum

unwinding: bump version to fix naked_asm on Xous

With rust-lang#80608 the `unwinding` crate no longer builds. The upstream crate has been updated to build by manually adding directives to the naked_asm stream.

Bump the dependency in Rust to get this newer version. This fixes the build for Xous, and closes rust-lang#134403.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS C-discussion Category: Discussion or questions that doesn't represent real issues. F-naked_functions `#![feature(naked_functions)]` O-xous OS: A microkernel OS for privacy in computing 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.

6 participants