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

Incorrect code generated from inline assembly with inout register on riscv32im-unknown-none-elf #128212

Closed
shkoo opened this issue Jul 25, 2024 · 6 comments · Fixed by #127513
Closed
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes O-riscv Target: RISC-V architecture P-high High priority

Comments

@shkoo
Copy link
Contributor

shkoo commented Jul 25, 2024

I tried compiling this code with a target of riscv32im-unknown-none-elf:

#![no_std]

static mut MY_BUFFER: [u32; 4] = [0u32; 4];

#[no_mangle]
unsafe fn using_inout() {
    let mut start = MY_BUFFER.as_mut_ptr();
    ::core::arch::asm!(
            "ecall",
            inout("a0") start);
    _ = start;
}

The assembly generated is incorrect, since it doesn't add in the lower 16 bits of the address of MY_BUFFER before calling the ecall:

using_inout:
        lui     a0, %hi(example::MY_BUFFER::he36bd30b506a9b69)
        ecall
        ret

As a workaround, changing the inout to inlateout seems to fix the problem:

using_inlateout:
        lui     a0, %hi(example::MY_BUFFER::he36bd30b506a9b69)
        addi    a0, a0, %lo(example::MY_BUFFER::he36bd30b506a9b69)
        ecall
        ret

Here is a compiler explorer link for rust nightly that demonstrates the problem:

https://godbolt.org/z/xMjoTo5oY

(For reference in case it gets fixed in nightly, it's also present in rust 1.80.0)

@shkoo shkoo added the C-bug Category: This is a bug. label Jul 25, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 25, 2024
@tgross35 tgross35 added O-riscv Target: RISC-V architecture A-inline-assembly Area: Inline assembly (`asm!(…)`) labels Jul 25, 2024
@tgross35
Copy link
Contributor

I haven't looked into this too much yet, but do you know if it reproduces using C's inline assembly fed through clang? This seems like it could be an LLVM problem.

@flaub
Copy link
Contributor

flaub commented Jul 26, 2024

It appears that this bug starts to appear in 1.77.

Using riscv32i-unknown-none-elf target:

1.76: https://godbolt.org/z/bKosKqvfW
1.77: https://godbolt.org/z/MYacc9na7

@flaub
Copy link
Contributor

flaub commented Jul 26, 2024

Could it be that this is the same underlying issue?
#126221

@tgross35
Copy link
Contributor

Did you mean between 1.77 and 1.78? The two links look the same to me.

It looks like this has to be a regression in LLVM, which was bumped to version 18 in 1.78, because we seem to generate the exact same IR. Could you file an issue with LLVM?

https://godbolt.org/z/Te9efcnMz

Could it be that this is the same underlying issue? #126221

It looks like it, I will close that one because this one has more detail.

@tgross35 tgross35 added WG-llvm Working group: LLVM backend code generation and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jul 26, 2024
@nikic nikic added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed WG-llvm Working group: LLVM backend code generation labels Jul 26, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 26, 2024
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

Being an unsound issue, maybe let's see what we can do. Compile target is Tier 2.

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 26, 2024
flaub added a commit to risc0/risc0 that referenced this issue Jul 26, 2024
flaub added a commit to risc0/risc0 that referenced this issue Jul 26, 2024
SchmErik added a commit to risc0/risc0 that referenced this issue Jul 26, 2024
See: rust-lang/rust#128212

---------

Co-authored-by: Erik Kaneda <erik@risczero.com>
SchmErik pushed a commit to risc0/risc0 that referenced this issue Jul 26, 2024
SchmErik pushed a commit to risc0/risc0 that referenced this issue Jul 26, 2024
@tgross35 tgross35 added the llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes label Jul 27, 2024
@tgross35
Copy link
Contributor

Fix applied to LLVM 19.x llvm/llvm-project#100843

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 31, 2024
Update to LLVM 19

The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.

The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by rust-lang#126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.

Compatibility note for wasm:

> WebAssembly target support for the `multivalue` target feature has changed when upgrading to LLVM 19. Support for generating functions with multiple returns no longer works and `-Ctarget-feature=+multivalue` has a different meaning than it did in LLVM 18 and prior. The WebAssembly target features `multivalue` and `reference-types` are now both enabled by default, but generated code is not affected by default. These features being enabled are encoded in the `target_features` custom section and may affect downstream tooling such as `wasm-opt` consuming the module, but the actual generated WebAssembly will continue to not use either `multivalue` or `reference-types` by default. There is no longer any supported means to generate a module that has a function with multiple returns.

Related changes:
 * rust-lang#127605
 * rust-lang#127613
 * rust-lang#127654
 * rust-lang#128141
 * llvm/llvm-project#98933

Fixes rust-lang#121444.
Fixes rust-lang#128212.
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 31, 2024
Update to LLVM 19

The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.

The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by rust-lang#126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.

Compatibility note for wasm:

> WebAssembly target support for the `multivalue` target feature has changed when upgrading to LLVM 19. Support for generating functions with multiple returns no longer works and `-Ctarget-feature=+multivalue` has a different meaning than it did in LLVM 18 and prior. The WebAssembly target features `multivalue` and `reference-types` are now both enabled by default, but generated code is not affected by default. These features being enabled are encoded in the `target_features` custom section and may affect downstream tooling such as `wasm-opt` consuming the module, but the actual generated WebAssembly will continue to not use either `multivalue` or `reference-types` by default. There is no longer any supported means to generate a module that has a function with multiple returns.

Related changes:
 * rust-lang#127605
 * rust-lang#127613
 * rust-lang#127654
 * rust-lang#128141
 * llvm/llvm-project#98933

Fixes rust-lang#121444.
Fixes rust-lang#128212.
@bors bors closed this as completed in 0b5eb7b Jul 31, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Aug 2, 2024
Update to LLVM 19

The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.

The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by rust-lang/rust#126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.

Compatibility note for wasm:

> WebAssembly target support for the `multivalue` target feature has changed when upgrading to LLVM 19. Support for generating functions with multiple returns no longer works and `-Ctarget-feature=+multivalue` has a different meaning than it did in LLVM 18 and prior. The WebAssembly target features `multivalue` and `reference-types` are now both enabled by default, but generated code is not affected by default. These features being enabled are encoded in the `target_features` custom section and may affect downstream tooling such as `wasm-opt` consuming the module, but the actual generated WebAssembly will continue to not use either `multivalue` or `reference-types` by default. There is no longer any supported means to generate a module that has a function with multiple returns.

Related changes:
 * rust-lang/rust#127605
 * rust-lang/rust#127613
 * rust-lang/rust#127654
 * rust-lang/rust#128141
 * llvm/llvm-project#98933

Fixes rust-lang/rust#121444.
Fixes rust-lang/rust#128212.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Aug 13, 2024
Update to LLVM 19

The LLVM 19.1.0 final release is planned for Sep 3rd. The rustc 1.82 stable release will be on Oct 17th.

The unstable MC/DC coverage support is temporarily broken by this update. It will be restored by rust-lang/rust#126733. The implementation changed substantially in LLVM 19, and there are no plans to support both the LLVM 18 and LLVM 19 implementation at the same time.

Compatibility note for wasm:

> WebAssembly target support for the `multivalue` target feature has changed when upgrading to LLVM 19. Support for generating functions with multiple returns no longer works and `-Ctarget-feature=+multivalue` has a different meaning than it did in LLVM 18 and prior. The WebAssembly target features `multivalue` and `reference-types` are now both enabled by default, but generated code is not affected by default. These features being enabled are encoded in the `target_features` custom section and may affect downstream tooling such as `wasm-opt` consuming the module, but the actual generated WebAssembly will continue to not use either `multivalue` or `reference-types` by default. There is no longer any supported means to generate a module that has a function with multiple returns.

Related changes:
 * rust-lang/rust#127605
 * rust-lang/rust#127613
 * rust-lang/rust#127654
 * rust-lang/rust#128141
 * llvm/llvm-project#98933

Fixes rust-lang/rust#121444.
Fixes rust-lang/rust#128212.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness llvm-fixed-upstream Issue expected to be fixed by the next major LLVM upgrade, or backported fixes O-riscv Target: RISC-V architecture P-high High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants