Skip to content

Fix RISC-V Test Failures in ./x test for Multiple Codegen and Assembly Cases #143915

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

CaiWeiran
Copy link

Resumption of #143792.
This PR addresses and resolves several test failures that occurred when running ./x test on the RISC-V architecture. The issues were due to platform-specific behavior, ABI differences, or code generation inconsistencies specific to RISC-V.

The following tests have been fixed:

  • assembly/dwarf-mixed-versions-lto.rs
  • codegen/const-vector.rs
  • codegen/enum/enum-aggregate.rs
  • codegen/simd/extract-insert-dyn.rs
  • codegen/transmute-scalar.rs
  • codegen/uninhabited-transparent-return-abi.rs

All changes have been tested locally with ./x test on a RISC-V target and now pass as expected.

Notes:

  • These fixes are scoped specifically to enable full test suite compliance for RISC-V targets.
  • No changes were made that affect other architectures.

Fix tests/assembly/dwarf-mixed-versions-lto.rs test failure on riscv64.
Fix tests/codegen/const-vector.rs test failure on riscv64.
Fix tests/codegen/enum/enum-aggregate.rs test failure on riscv64.
Fix tests/codegen/simd/extract-insert-dyn.rs test failure on riscv64.
Fix tests/codegen/transmute-scalar.rs test failure on riscv64.
Fix tests/codegen/uninhabited-transparent-return-abi.rs test failure on riscv64.
@rustbot
Copy link
Collaborator

rustbot commented Jul 14, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 14, 2025
@CaiWeiran
Copy link
Author

@Mark-Simulacrum I have merged different revisions into a single file.

@rust-log-analyzer

This comment has been minimized.

Fix tidy check error.
@CaiWeiran CaiWeiran changed the title Ci test Fix RISC-V Test Failures in ./x test for Multiple Codegen and Assembly Cases Jul 14, 2025
@rust-log-analyzer

This comment has been minimized.

Fix regex matching error.
@@ -114,7 +114,7 @@ fn make_uninhabited_err_indirectly(n: Never) -> Result<u32, Never> {
fn make_fully_uninhabited_result(v: u32, n: Never) -> Result<(u32, Never), (Never, u32)> {
// Actually reaching this would be UB, so we don't actually build a result.

// CHECK-LABEL: { i32, i32 } @make_fully_uninhabited_result(i32 %v)
// CHECK-LABEL: { i32, i32 } @make_fully_uninhabited_result(i32 {{.*}}%v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the actual content we're hiding here (and presumably the same elsewhere)? i.e., what attributes are unique to riscv?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

riscv: CHECK-LABEL: { i32, i32 } @make_fully_uninhabited_result(i32 signext %v)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I guess riscv falls in the second case:

If an integer argument to a function is not marked signext/zeroext/noext, the kind of extension used is target-specific. Some targets depend for correctness on the kind of extension to be explicitly specified.

I'm not sure if this is the best way to handle this case (maybe we should have two versions of the CHECK statement) or have some normalization outside of each individual test.

cc @scottmcm (I feel like you look at codegen tests a lot)
@rustbot ping llvm

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using #[cfg(target_arch = "xxx")], but it didn't work.

@rustbot
Copy link
Collaborator

rustbot commented Jul 15, 2025

Error: This team (llvm) cannot be pinged via this command; it may need to be added to triagebot.toml on the default branch.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Mark-Simulacrum
Copy link
Member

cc @nikic for whether there's a better pattern for us to follow in codegen tests to normalize/ignore the per-target ABI questions (E.g., signext above), or if we should instead retain test coverage very explicitly of them.

@nikic
Copy link
Contributor

nikic commented Jul 15, 2025

@Mark-Simulacrum Ignoring ABI attributes via {{.*}} seems fine. But I think that for tests like transmute-scalar.rs that aren't testing anything target specific, we should be specifying an explicit --target to avoid these issues. I feel like that might have been the intention for that test all along, seeing as how it uses minicore already.

@CaiWeiran
Copy link
Author

@Mark-Simulacrum Ignoring ABI attributes via {{.*}} seems fine. But I think that for tests like transmute-scalar.rs that aren't testing anything target specific, we should be specifying an explicit --target to avoid these issues. I feel like that might have been the intention for that test all along, seeing as how it uses minicore already.

@nikic I'm not sure I understand. If I don't make any changes, running ./x test --target riscv64gc-unknown-linux-gnu tests/codegen/enum/enum-aggregate.rs still fails. So even when specifying --target, the test fails due to target-specific ABI differences.

@nikic
Copy link
Contributor

nikic commented Jul 16, 2025

@Mark-Simulacrum Ignoring ABI attributes via {{.*}} seems fine. But I think that for tests like transmute-scalar.rs that aren't testing anything target specific, we should be specifying an explicit --target to avoid these issues. I feel like that might have been the intention for that test all along, seeing as how it uses minicore already.

@nikic I'm not sure I understand. If I don't make any changes, running ./x test --target riscv64gc-unknown-linux-gnu tests/codegen/enum/enum-aggregate.rs still fails. So even when specifying --target, the test fails due to target-specific ABI differences.

The test should specify

//@ compile-flags: --target=x86_64-unknown-linux-gnu
//@ needs-llvm-components: x86

To always run against the x86 triple even if you are running on a riscv host.

@CaiWeiran
Copy link
Author

CaiWeiran commented Jul 16, 2025

@Mark-Simulacrum Ignoring ABI attributes via {{.*}} seems fine. But I think that for tests like transmute-scalar.rs that aren't testing anything target specific, we should be specifying an explicit --target to avoid these issues. I feel like that might have been the intention for that test all along, seeing as how it uses minicore already.

@nikic I'm not sure I understand. If I don't make any changes, running ./x test --target riscv64gc-unknown-linux-gnu tests/codegen/enum/enum-aggregate.rs still fails. So even when specifying --target, the test fails due to target-specific ABI differences.

The test should specify

//@ compile-flags: --target=x86_64-unknown-linux-gnu
//@ needs-llvm-components: x86

To always run against the x86 triple even if you are running on a riscv host.

@nikic It will fail the test if I don't install the x86_64-unknown-linux-gnu target on the RISC-V host.
When running tests on a RISC-V host, my goal is to test the RISC-V triple.
If a certain test is specifically for x86, would it be better to mark it with //@ only-x86?
Or alternatively, modify the matching rules like I did to adapt to different ISAs?

@nikic
Copy link
Contributor

nikic commented Jul 16, 2025

@CaiWeiran Using --target + needs-llvm-components + no_core ensures that the test can run on arbitrary hosts. It should not fail on RISCV if everything is implemented correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants