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

llvm wrapper's target feature filtering breaks UI tests, is difficult to debug #114661

Open
sethp opened this issue Aug 9, 2023 · 0 comments
Open
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@sethp
Copy link
Contributor

sethp commented Aug 9, 2023

Every attempt to ask LLVM "does the target support this feature?" either succeeds or causes a side effect of writing a message to stderr:

  // If there is a match
  if (FeatureEntry) {
    // ...   
  } else {
    errs() << "'" << Feature << "' is not a recognized feature for this target"
           << " (ignoring feature)\n";
  }

Since the UI tests are very strict about matching exactly the same stderr output across LLVM versions, this means teaching rustc about an LLVM feature that exists in only some versions but not others creates a contradiction: if that error line is expected, the tests will fail against newer versions of LLVM, and if it's absent then the tests can't pass against the older versions. Since rustc runs the filter at target selection time, any UI tests at all for this architecture will fail, regardless of their contents.

What's worse is the developer experience. I ran into it introducing UI tests for riscv targets as part of adding an ABI (see #111891 (comment) ) and since the issue presents as having forgotten to --bless some output, I went back and forth a number of times adding and removing the line. It's not at all clear from the failure that an unused feature supported-on-some-but-not-other-LLVMs is the culprit, and because it fails in CI the feedback loop for this condition is terribly long.

Some possible resolutions include:

  1. Remove the preemptive LLVM target feature filtering entirely and rely on LLVM to ignore unknown features; this matches the current state of the codebase, in that it's not possible to get CI to approve a version of rustc that relies on the filtering, so it's effectively a no-op.
  2. Plan to remove, suspend, or modify UI tests for any architectures that have partial feature support when new features are added; the challenge here would be decidedly DX shaped in effectively communicating this plan to anyone touching these tests who might not be otherwise aware of it (such as myself).
  3. Remove the side effect from the filter code path. The only workable implementation of this approach that I found is in 70b2188 ; the carrying cost there can be time-bounded by asking upstream LLVM to add a getter and then waiting for that change to percolate all throughout the various packaging ecosystems. That's work I'm willing to take on, if that would make this change acceptable to repair rustc in the meantime.

/cc @nikic

@sethp sethp added the C-bug Category: This is a bug. label Aug 9, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 9, 2023
@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants