Skip to content

organize and extend forbidden target feature tests #140395

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

Merged
merged 3 commits into from
May 3, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 28, 2025

In particular this adds some loongarch tests for #135015, Cc @heiher

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.

try-job: x86_64-gnu-llvm-20-*

@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2025

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 Apr 28, 2025
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r=me with tidy fixed.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 28, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@RalfJung RalfJung force-pushed the target-feature-tests branch from 1b9008f to 240cc10 Compare April 28, 2025 14:09
@rust-log-analyzer

This comment has been minimized.

|
= note: this feature is not stably supported; its behavior can change in the future

warning: both target-abi and the triple-implied ABI are invalid, ignoring and using feature-implied ABI
Copy link
Member Author

@RalfJung RalfJung Apr 28, 2025

Choose a reason for hiding this comment

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

@heiher any idea what this warning is about? Does it come from LLVM?

For riscv it says

Hard-float 'd' ABI can't be used for a target that doesn't support the D instruction set extension (ignoring target-abi)

which is a lot more clear. Is this trying to say the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are quite similar. The LoongArch backend attempts to compute the ABI based on the target-abi and triple-name. If there’s a conflict between the resulting ABI and the target features, the final ABI will be determined by the target features.

Fore more details:

https://github.com/rust-lang/llvm-project/blob/rustc-1.86.0/llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchBaseInfo.cpp#L171-L172

Copy link
Member Author

@RalfJung RalfJung Apr 29, 2025

Choose a reason for hiding this comment

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

Okay. I find the LoongArch error quite unclear compared to the RISC-V error... but anyway it should be impossible to trigger this warning from rustc without also triggering "target feature d must be enabled to ensure that the ABI of the current target can be implemented correctly" so users will hopefully understand what the problem is.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 28, 2025

So somehow on CI we get a bunch of extra warnings:

2025-04-28T14:35:22.9208959Z '+div32' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9209370Z '+div32' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9209778Z '+lam-bh' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9210178Z '+lam-bh' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9210688Z '+lamcas' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211099Z '+lamcas' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211515Z '+ld-seq-sa' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211932Z '+ld-seq-sa' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9212335Z '+scq' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9212721Z '+scq' is not a recognized feature for this target (ignoring feature)

I don't get those when running the test locally. Is that an LLVM version thing? Does loongarch64-unknown-none not work on older LLVM versions (not without a bunch of warnings, anyway)? How would one write a test for that target then...? And what even enables those target features?!?

@heiher
Copy link
Contributor

heiher commented Apr 29, 2025

So somehow on CI we get a bunch of extra warnings:

2025-04-28T14:35:22.9208959Z '+div32' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9209370Z '+div32' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9209778Z '+lam-bh' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9210178Z '+lam-bh' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9210688Z '+lamcas' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211099Z '+lamcas' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211515Z '+ld-seq-sa' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9211932Z '+ld-seq-sa' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9212335Z '+scq' is not a recognized feature for this target (ignoring feature)
2025-04-28T14:35:22.9212721Z '+scq' is not a recognized feature for this target (ignoring feature)

I don't get those when running the test locally. Is that an LLVM version thing? Does loongarch64-unknown-none not work on older LLVM versions (not without a bunch of warnings, anyway)? How would one write a test for that target then...? And what even enables those target features?!?

Thank you for everything you've done.

IIRC, when the LLVM backend doesn't support target features listed in compiler/rustc_target/src/target_features.rs, the rustc throws these kinds of warnings. For LoongArch, features like div32 were introduced starting from LLVM 20, so they are not supported in LLVM 19, which is expected behavior.

Would it be possible to add a min-llvm-version to the test cases? This would help to handle the differences between LLVM version.

diff --git a/tests/ui/target-feature/abi-required-target-feature-flag-disable.rs b/tests/ui/target-feature/abi-required-target-feature-flag-disable.rs
index 02841b6690a..5b0d7b7b252 100644
--- a/tests/ui/target-feature/abi-required-target-feature-flag-disable.rs
+++ b/tests/ui/target-feature/abi-required-target-feature-flag-disable.rs
@@ -12,6 +12,7 @@
 //@[riscv] needs-llvm-components: riscv
 //@[loongarch] compile-flags: --target=loongarch64-unknown-none -Ctarget-feature=-d
 //@[loongarch] needs-llvm-components: loongarch
+//@[loongarch] min-llvm-version: 20
 // For now this is just a warning.
 //@ build-pass

It would also be useful for the warning: Hard-float 'd' ABI can't be used ... for RISC-V.

@RalfJung
Copy link
Member Author

IIRC, when the LLVM backend doesn't support target features listed in compiler/rustc_target/src/target_features.rs, the rustc throws these kinds of warnings.

I'm surprised by this. I thought the warning should should only show up when the target feature is actually used, but I did a quick test and you are right. I hope these warnings do not show up on stable?

We may have to come up with a better way to deal with target features that are not supported by all of the LLVM versions we support, the current approach feels fragile.

It would also be useful for the warning: Hard-float 'd' ABI can't be used ... for RISC-V.

I don't think so, that warning appears consistently for all LLVM versions it seems (so it's not a problem).

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

Oh I see what you mean, that RISCV error didn't yet show in LLVM 19.

@RalfJung RalfJung force-pushed the target-feature-tests branch from f78cc1b to 5b7e669 Compare April 29, 2025 08:20
@RalfJung
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Apr 29, 2025

📌 Commit 5b7e669 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 29, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 29, 2025
…etrochenkov

organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc `@heiher`

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2025
Rollup of 6 pull requests

Successful merges:

 - rust-lang#139883 (crashes: more tests)
 - rust-lang#140312 (Improve pretty-printing of braces)
 - rust-lang#140392 (compiletest: Remove the libtest-based executor and its dependency)
 - rust-lang#140395 (organize and extend forbidden target feature tests)
 - rust-lang#140422 (unwind: bump `unwinding` dependency to 0.2.6)
 - rust-lang#140432 (Update documentation for `fn target_config`)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 30, 2025
…etrochenkov

organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc ``@heiher``

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
Rollup of 14 pull requests

Successful merges:

 - rust-lang#140380 (transmutability: uninit transition matches unit byte only)
 - rust-lang#140385 (Subtree update of `rust-analyzer`)
 - rust-lang#140395 (organize and extend forbidden target feature tests)
 - rust-lang#140430 (Improve test coverage of HIR pretty printing.)
 - rust-lang#140458 (Fix for async drop ice with partly dropped tuple)
 - rust-lang#140460 (Fix handling of LoongArch target features not supported by LLVM 19)
 - rust-lang#140465 (chore: edit and move tests)
 - rust-lang#140467 (Don't FCW assoc consts in patterns)
 - rust-lang#140468 (Minor tweaks to make some normalization (adjacent) code less confusing)
 - rust-lang#140470 (CI: rfl: move job forward to Linux v6.15-rc4)
 - rust-lang#140476 (chore: delete unused ui/auxiliary crates)
 - rust-lang#140481 (Require sanitizers be enabled for asan_odr_windows.rs)
 - rust-lang#140486 (rustfmt: Also allow bool literals as first item of let chain)
 - rust-lang#140494 (Parser: Document restrictions)

r? `@ghost`
`@rustbot` modify labels: rollup
Zalathar added a commit to Zalathar/rust that referenced this pull request Apr 30, 2025
…etrochenkov

organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc ```@heiher```

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.
@RalfJung
Copy link
Member Author

Hm no that can't be the issue, that only changes things for older LLVM...
@bors try

@nikic @heiher any idea why the LLVM 20 on the CI runner does not produce the warnings that I get when building these tests locally?

@bors
Copy link
Collaborator

bors commented Apr 30, 2025

⌛ Trying commit e90dafa with merge 5143e37...

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc `@heiher`

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.

try-job: `x86_64-gnu-llvm-*`
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 30, 2025

💔 Test failed - checks-actions

@RalfJung
Copy link
Member Author

RalfJung commented May 1, 2025

On the LLVM side, these messages seem to be output via errs() << .... Is that output maybe suppressed under some conditions? (That would honestly be kind of bad, at least some of them indicate critical issues.)

@taiki-e @nagisa maybe you have an idea about what LLVM is doing here?

@nagisa
Copy link
Member

nagisa commented May 2, 2025

In my experience you can't suppress error output from LLVM, no, but that's just experience from a long time ago. No idea if they maybe changed things up for LLVM 20+

@nikic
Copy link
Contributor

nikic commented May 2, 2025

I don't know why there would be a behavior difference between Rust's and Ubuntu's LLVM here.

How does rust intercept LLVM's stderr stream to produce these as warnings?

@RalfJung
Copy link
Member Author

RalfJung commented May 2, 2025

I don't know why there would be a behavior difference between Rust's and Ubuntu's LLVM here.

So these CI runners use the Ubuntu LLVM? Maybe they applied some patches...?

How does rust intercept LLVM's stderr stream to produce these as warnings?

Does it do that? I thought this is just LLVM printing directly to stderr. The warnings do not show up in rustc's warning count, for instance.

@nikic
Copy link
Contributor

nikic commented May 2, 2025

How does rust intercept LLVM's stderr stream to produce these as warnings?

Does it do that? I thought this is just LLVM printing directly to stderr. The warnings do not show up in rustc's warning count, for instance.

Oh, I saw the warning: prefixes and thought rustc is adding those. But apparently LLVM itself emits them in that format.

@nikic
Copy link
Contributor

nikic commented May 2, 2025

My best guess is that with these warnings being emitted in the backend, there might be some kind of threading interaction?

@RalfJung
Copy link
Member Author

RalfJung commented May 2, 2025

Well, I think I'll just try to normalize them away.

@RalfJung RalfJung force-pushed the target-feature-tests branch from e90dafa to 6873a46 Compare May 2, 2025 12:11
@RalfJung
Copy link
Member Author

RalfJung commented May 2, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request May 2, 2025
organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc `@heiher`

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.

try-job: `x86_64-gnu-llvm-20-*`
@bors
Copy link
Collaborator

bors commented May 2, 2025

⌛ Trying commit 6873a46 with merge 9081c65...

@bors
Copy link
Collaborator

bors commented May 2, 2025

☀️ Try build successful - checks-actions
Build commit: 9081c65 (9081c650eb0f67a8654540d94b56ad64895c8e3d)

@RalfJung
Copy link
Member Author

RalfJung commented May 2, 2025

@bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented May 2, 2025

📌 Commit 6873a46 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 2, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138712 (resolve: Support imports of associated types and glob imports from traits)
 - rust-lang#140395 (organize and extend forbidden target feature tests)
 - rust-lang#140576 (Remove fragile equal-pointers-unequal tests.)
 - rust-lang#140582 (Update sysinfo to `0.35.0` in bootstrap and `tools/opt-dist`)
 - rust-lang#140595 (doc(std): fix typo lchown -> lchmod)
 - rust-lang#140597 (zkvm: remove schmerik as target maintainer)
 - rust-lang#140604 (yet another small borrowck cleanup )

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d8b0347 into rust-lang:master May 3, 2025
7 checks passed
@rustbot rustbot added this to the 1.88.0 milestone May 3, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2025
Rollup merge of rust-lang#140395 - RalfJung:target-feature-tests, r=petrochenkov

organize and extend forbidden target feature tests

In particular this adds some loongarch tests for rust-lang#135015, Cc `@heiher`

Seems like the tests change so much git does not detect the renames; a commit-by-commit review should help.

try-job: `x86_64-gnu-llvm-20-*`
@RalfJung RalfJung deleted the target-feature-tests branch May 4, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants