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

compiletest: Remove the magic hacks for finding output with lto=thin #131524

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

Zalathar
Copy link
Contributor

This hack was intended to handle the case where -Clto=thin causes the compiler to emit multiple output files (when producing LLVM-IR or assembly).

The hack only affects 4 tests, of which 3 are just meta-tests for the hack itself. The one remaining test that motivated the hack currently doesn't even need it!

(tests/codegen/issues/issue-81408-dllimport-thinlto-windows.rs)

This hack was intended to handle the case where `-Clto=thin` causes the
compiler to emit multiple output files (when producing LLVM-IR or assembly).

The hack only affects 4 tests, of which 3 are just meta-tests for the hack
itself. The one remaining test that motivated the hack currently doesn't even
need it!

(`tests/codegen/issues/issue-81408-dllimport-thinlto-windows.rs`)
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Oct 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@Zalathar
Copy link
Contributor Author

Mostly reverts #118036, which was trying to fix a test failure observed in #113923.

I do think it's reasonable for compiletest to have some way of handling the multiple-output-file scenario for these test suites, but:

  • It should be opt-in (via a directive) or use a more principled condition, not just scanning compile-flags for lto=thin.
  • It needs to not cause maintenance headaches for compiletest in general and runtest.rs in particular.
  • It should only exist if there's actually a real use-case for it.

@Zalathar
Copy link
Contributor Author

I also considered removing //@ only-windows from that one remaining test, because it works fine on other host platforms, but I didn't want this PR to have to play CI whack-a-mole with wasm and other weird targets.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Ah cool, this thing was quite complex when I first read it. Thanks!

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 96224d8 has been approved by jieyouxu

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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 11, 2024
compiletest: Simplify the choice of `--emit` mode for assembly tests

Tiny little cleanup that I noticed while working on rust-lang#131524. No functional change.

Historically, the original code structure (rust-lang#58791) predates the `Emit` enum (rust-lang#103298), so it was manually adding `--emit` flags to the compiler invocation. But now the match can just evaluate to the appropriate `Emit` value directly.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
Rollup of 3 pull requests

Successful merges:

 - rust-lang#131305 (make `llvm::is_ci_llvm_modified` logic more precise)
 - rust-lang#131524 (compiletest: Remove the magic hacks for finding output with `lto=thin`)
 - rust-lang#131525 (compiletest: Simplify the choice of `--emit` mode for assembly tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f6bdf71 into rust-lang:master Oct 11, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
Rollup merge of rust-lang#131525 - Zalathar:emit-asm, r=jieyouxu

compiletest: Simplify the choice of `--emit` mode for assembly tests

Tiny little cleanup that I noticed while working on rust-lang#131524. No functional change.

Historically, the original code structure (rust-lang#58791) predates the `Emit` enum (rust-lang#103298), so it was manually adding `--emit` flags to the compiler invocation. But now the match can just evaluate to the appropriate `Emit` value directly.
@rustbot rustbot added this to the 1.83.0 milestone Oct 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
Rollup merge of rust-lang#131524 - Zalathar:less-thinlto-magic, r=jieyouxu

compiletest: Remove the magic hacks for finding output with `lto=thin`

This hack was intended to handle the case where `-Clto=thin` causes the compiler to emit multiple output files (when producing LLVM-IR or assembly).

The hack only affects 4 tests, of which 3 are just meta-tests for the hack itself. The one remaining test that motivated the hack currently doesn't even need it!

(`tests/codegen/issues/issue-81408-dllimport-thinlto-windows.rs`)
@Zalathar Zalathar deleted the less-thinlto-magic branch October 11, 2024 09:45
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 A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Development

Successfully merging this pull request may close these issues.

5 participants