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

Run Miri and mir-opt tests without a target linker #119035

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Dec 17, 2023

Normally, we need a linker for the target to build the standard library. That's only because std declares crate-type lib and dylib; building the dylib is what creates a need for the linker.

But for mir-opt tests (and for Miri) we do not need to build a libstd.so. So with this PR, when we build the standard library for mir-opt tests, instead of cargo build we run cargo rustc --crate-type=lib which overrides the configured crate types in std's manifest.

I've also swapped in what seems to me a better hack than BOOTSTRAP_SKIP_TARGET_SANITY to prevent cross-interpreting with Miri from checking for a target linker and expanded it to mir-opt tests too. Whether it's actually better is up to a reviewer.

@rustbot rustbot added 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 Dec 17, 2023
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 30, 2023
@rust-log-analyzer

This comment has been minimized.

@saethlin saethlin force-pushed the remove-linker-requirement branch 4 times, most recently from 1397d2e to 3af811e Compare January 1, 2024 03:32
@saethlin saethlin marked this pull request as ready for review January 1, 2024 04:28
@saethlin saethlin changed the title [WIP] Run Miri and mir-opt tests without a target linker Run Miri and mir-opt tests without a target linker Jan 1, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2024

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@saethlin
Copy link
Member Author

saethlin commented Jan 1, 2024

r? bootstrap

@saethlin
Copy link
Member Author

saethlin commented Jan 1, 2024

Oops this still contains #119475. I should fix that.

@saethlin
Copy link
Member Author

saethlin commented Jan 2, 2024

Fixed

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Changes looks good to me. Feel free to r=me when you want to queue it up.

src/bootstrap/src/core/sanity.rs Outdated Show resolved Hide resolved
@onur-ozkan
Copy link
Member

@bors rollup=iffy

@saethlin
Copy link
Member Author

saethlin commented Jan 2, 2024

@onur-ozkan I realized I goofed a bit right around the diff you commented on, can you take a look again? (no rush of course)

@onur-ozkan
Copy link
Member

@onur-ozkan I realized I goofed a bit right around the diff you commented on, can you take a look again? (no rush of course)

Seems good to me, even better with bitwise OR :)

@saethlin
Copy link
Member Author

saethlin commented Jan 3, 2024

@bors r=onur-ozkan

@bors
Copy link
Contributor

bors commented Jan 3, 2024

📌 Commit 1abf9ea has been approved by onur-ozkan

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 3, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Jan 3, 2024
} else {
let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "build");
std_cargo(builder, target, compiler.stage, &mut cargo);
for krate in &*self.crates {
Copy link
Member

Choose a reason for hiding this comment

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

So self.crates is entirely ignored when is_for_mir_opt_tests is set?

Copy link
Member Author

@saethlin saethlin Jan 3, 2024

Choose a reason for hiding this comment

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

Yes; as far as I understand crates is the crates you're asking to build explicitly. For example when you run x build library/core crates is ["core"]. When we're building (possibly cross-compiling) a sysroot for mir-opt tests, the only thing it makes sense to build is std and all of its dependencies; attempting to select any other set would be unnecessary if you're explicitly requesting library/test, or insufficient to build some of the mir-opt tests if you tried to request only core.

Copy link
Member

@RalfJung RalfJung Jan 3, 2024

Choose a reason for hiding this comment

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

When we're building (possibly cross-compiling) a sysroot for mir-opt tests, the only thing it makes sense to build is std and all of its dependencies

What about the "proc_macro" crate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

And somehow it also works for Miri... 🤷 whatever, as long as CI is happy.^^ I just hope I won't have to debug this, the bootstrap logic is getting more and more messy it feels like.

Copy link
Member Author

@saethlin saethlin Jan 4, 2024

Choose a reason for hiding this comment

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

Miri doesn't use this code path, the only change for Miri is how we detect that we should disable the target sanity check.

The logic here is complicated mostly because the mir-opt tests cannot reuse Miri's technique for building a MIR-only sysroot because of this call:

if !tcx.is_mir_available(callee.def_id()) {
, a check build doesn't produce MIR, a check build with -Zalways-encode-mir produces sporadic changes to inlining cyclic calls, and a normal build needs a linker to produce libstd.so. So we need a normal build but one that disables the dylib crate-type for std, and cargo rustc --crate-type seems to be the only option and also designed for this based on the RFC.

I agree that the code is rather complicated, but I think that reflects our overall poor support for cross compilation as opposed to being new complexity. And I'm volunteering to help maintain this sort of code; I understand it now that I've worked on it a bit.

@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 Jan 6, 2024
@mati865
Copy link
Contributor

mati865 commented Jan 6, 2024

If it fails again you might try running it as try build, IIRC they have config that is closer to final builds.

@saethlin
Copy link
Member Author

saethlin commented Jan 6, 2024

@mati865 Unfortunately I don't think that would help; the bors failure was a test not passing on x86_64-mingw. The dist jobs didn't fail (not that they managed to finish before being cancelled either).

@bors r=onur-ozkan

@bors
Copy link
Contributor

bors commented Jan 6, 2024

📌 Commit 735a6a4 has been approved by onur-ozkan

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 6, 2024

⌛ Testing commit 735a6a4 with merge 6f7cb43...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2024
…r=onur-ozkan

Run Miri and mir-opt tests without a target linker

Normally, we need a linker for the target to build the standard library. That's only because `std` declares crate-type lib and dylib; building the dylib is what creates a need for the linker.

But for mir-opt tests (and for Miri) we do not need to build a `libstd.so`. So with this PR, when we build the standard library for mir-opt tests, instead of `cargo build` we run `cargo rustc --crate-type=lib` which overrides the configured crate types in `std`'s manifest.

I've also swapped in what seems to me a better hack than `BOOTSTRAP_SKIP_TARGET_SANITY` to prevent cross-interpreting with Miri from checking for a target linker and expanded it to mir-opt tests too. Whether it's actually better is up to a reviewer.
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jan 6, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 6, 2024
@saethlin
Copy link
Member Author

saethlin commented Jan 6, 2024

warning: spurious network error (1 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
  error: failed to download from `[https://crates.io/api/v1/crates/ppv-lite86/0.2.17/download`](https://crates.io/api/v1/crates/ppv-lite86/0.2.17/download%60)
  
  Caused by:
    [6] Couldn't resolve host name (Could not resolve host: crates.io)
  Build completed unsuccessfully in 0:11:19
    local time: Sat Jan  6 22:32:56 UTC 2024
    network time: Sat, 06 Jan 2024 22:32:56 GMT

@bors retry

@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 Jan 6, 2024
@bors
Copy link
Contributor

bors commented Jan 7, 2024

⌛ Testing commit 735a6a4 with merge 78c988f...

@bors
Copy link
Contributor

bors commented Jan 7, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 78c988f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2024
@bors bors merged commit 78c988f into rust-lang:master Jan 7, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 7, 2024
@saethlin saethlin deleted the remove-linker-requirement branch January 7, 2024 02:34
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (78c988f): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.8% [3.8%, 3.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.8% [3.8%, 3.8%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
3.7% [3.7%, 3.7%] 1
Regressions ❌
(secondary)
2.1% [2.0%, 2.2%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.7% [3.7%, 3.7%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.404s -> 666.535s (-0.13%)
Artifact size: 308.54 MiB -> 308.52 MiB (-0.01%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
…wesleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
…wesleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…wesleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
…sleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
…sleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
…sleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
…sleywiser

Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Feb 8, 2024
Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in rust-lang/rust#119035, but not the effect. Sorry for the churn, I've learned a lot about how all this works over the past few weeks.

The objective here is to make `x test mir-opt --bless` use the same set of targets on all platforms. It didn't do that from the start because bootstrap assumes that a target linker is available, so the availability of cross-linkers is how we ended up with `MIR_OPT_BLESS_TARGET_MAPPING` and poor support for blessing mir-opt tests from Aarch64 MacOS. This PR corrects that.

So I've adjusted the bless targets for mir-opt tests, as well as tweaked some of the logic in bootstrap about linker configuration so that we don't try to access the cache of cc/linker configuration when doing the mir-opt builds.

While working on that I realized that if I swapped from the `cargo rustc -p std` strategy to `cargo check` on the sysroot, I could use the existing code for check builds to bypass some linker logic. Sweet.

But just doing that doesn't work, because then mir-opt tests complain that they can't find an rlib for any of the standard library crates. That happens because nearly all the mir-opt tests are attempting to build `CrateType::Executable`. We already have all the MIR required for mir-opt tests from the rmeta files, but since rustc think we're trying to build an executable it demands we have access to all the upstream monomorphizations that only exist in rlibs, not the meta files in a MIR-only sysroot.

So to fix that, I've swapped all the mir-opt tests be passed `--crate-type=rlib`. That works, but leaves us with a few broken mir-opt tests which I've blessed or fixed up; we also lose MIR for some functions so I added `-Clink-dead-code` to paper over that. The inlining changes are because changing the crate-type perturbs the hashes that are compared here to sometimes let us do inlining even in a possibly-recursive call: https://github.com/rust-lang/rust/blob/4cb17b4e78e0540e49d2da884cc621a6bf6f47fa/compiler/rustc_mir_transform/src/inline.rs#L332-L341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants