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

//@ needs-llvm-components accepts invalid LLVM components #138145

Open
tgross35 opened this issue Mar 7, 2025 · 6 comments
Open

//@ needs-llvm-components accepts invalid LLVM components #138145

tgross35 opened this issue Mar 7, 2025 · 6 comments
Labels
A-compiletest Area: The compiletest test runner C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@tgross35
Copy link
Contributor

tgross35 commented Mar 7, 2025

For example, a codegen test:

//@ needs-llvm-components: riscv64
//@ compile-flags: --target riscv64gc-unknown-linux-gnu

#![crate_type = "lib"]
#![no_std]
#![no_core]
#![feature(no_core, lang_items)]
extern crate minicore;

// CHECK: fail!

riscv64 isn't a real LLVM component (only riscv). However, the test never errors; instead it always gets ignored. It seems pretty likely that there are at least a few in-tree tests that never get run because of a typo here.

It should be reasonably easy to keep a list of valid LLVM components and validate against that in compiletest.

@tgross35 tgross35 added A-compiletest Area: The compiletest test runner C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Mar 7, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 7, 2025
@jieyouxu jieyouxu added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Mar 7, 2025
@jieyouxu
Copy link
Member

jieyouxu commented Mar 7, 2025

Doesn't the list of valid llvm components change w/ the underlying llvm? Like you can build a local llvm that do not have what CI llvm has

@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 7, 2025
@tgross35
Copy link
Contributor Author

tgross35 commented Mar 7, 2025

I'm imagining this would be a list of components somewhere in compiletest rather than querying LLVM, the set is pretty small and static. The built LLVM can have components turned on or off but it will always be a subset of that list; if the component isn't available on the current LLVM version then we ignore the test, but if the component isn't available at all then it would be better to error.

That might not be accurate if "component" ever refers to something other than an architecture family, but I don't see any indication of that or use in our tests. It seems like it corresponds to the options from LLVM_TARGETS_TO_BUILD, which are listed as AArch64;AMDGPU;ARM;AVR;BPF;Hexagon;Lanai;LoongArch;Mips;MSP430;NVPTX;PowerPC;RISCV;Sparc;SystemZ;VE;WebAssembly;X86;XCore at https://llvm.org/docs/CMake.html.

@s7tya
Copy link
Contributor

s7tya commented Mar 7, 2025

Hi. I found a similar attempt to check invalid component names in lint, but it was later reverted in #125949.
From what I understand, based on that previous attempt, the names specified in needs-llvm-components are already validated by CI when COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS is set. I might be mistaken, but how does this issue differ from that previous attempt?

@jieyouxu
Copy link
Member

jieyouxu commented Mar 7, 2025

No you're right, this will indeed fail in CI if the component does not exist. But as in, CI will require that all tests with the components that CI LLVM is built with is run.

I suppose one may be able to query llvm-config --components to validate that the component exists, or somehow pull out the targets to build list elsewhere to share.

@tgross35
Copy link
Contributor Author

tgross35 commented Mar 7, 2025

Oh awesome, I didn't realize this did get checked in CI. It would still be nice to have something for development; could we bless the output from llvm-config to a file when COMPILETEST_REQUIRE_ALL_LLVM_COMPONENTS is set and check against that?

@jieyouxu
Copy link
Member

jieyouxu commented Mar 7, 2025

Possibly? It'd somehow need to be not tracked to prevent you from accidentally checking that in.

@jieyouxu jieyouxu added the E-needs-design This issue needs exploration and design to see how and if we can fix/implement it label Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

No branches or pull requests

4 participants