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

saethlin · 2023-12-17T04:11:12Z

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 · 2024-01-01T04:29:00Z

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 · 2024-01-01T04:29:38Z

r? bootstrap

saethlin · 2024-01-01T04:43:29Z

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

saethlin · 2024-01-02T11:03:19Z

Fixed

onur-ozkan

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

src/bootstrap/src/core/sanity.rs

onur-ozkan · 2024-01-02T16:39:58Z

@bors rollup=iffy

saethlin · 2024-01-02T22:46:17Z

@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 · 2024-01-02T23:25:21Z

@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 · 2024-01-03T01:05:53Z

@bors r=onur-ozkan

bors · 2024-01-03T01:05:56Z

📌 Commit 1abf9ea has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-01-03T01:05:57Z

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

RalfJung · 2024-01-03T21:19:13Z

src/bootstrap/src/core/build_steps/compile.rs

+        } 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 {


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

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.

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?

That's handled down here: https://github.com/rust-lang/rust/pull/119035/files#diff-675065335a90de4a4e1ac5bc0a078709f865ff2d357ba2650309f0ad8614b1d3R1620-R1621

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.

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:

rust/compiler/rustc_mir_transform/src/inline/cycle.rs

Line 76 in 090d5ea

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.

mati865 · 2024-01-06T20:17:27Z

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

saethlin · 2024-01-06T20:27:33Z

@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 · 2024-01-06T20:27:35Z

📌 Commit 735a6a4 has been approved by onur-ozkan

It is now in the queue for this repository.

bors · 2024-01-06T22:18:27Z

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

…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 · 2024-01-06T22:33:13Z

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 · 2024-01-06T22:33:47Z

💔 Test failed - checks-actions

saethlin · 2024-01-06T22:49:19Z

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 · 2024-01-07T00:32:27Z

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

bors · 2024-01-07T02:29:57Z

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

rust-timer · 2024-01-07T03:50:02Z

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%)

…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

…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

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

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

This comment has been minimized.

Sign in to view

saethlin force-pushed the remove-linker-requirement branch from 5ae350d to 657158a Compare December 30, 2023 19:42

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

This comment has been minimized.

Sign in to view

saethlin force-pushed the remove-linker-requirement branch 4 times, most recently from 1397d2e to 3af811e Compare January 1, 2024 03:32

saethlin marked this pull request as ready for review January 1, 2024 04:28

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 assigned onur-ozkan Jan 1, 2024

saethlin force-pushed the remove-linker-requirement branch from 3af811e to 7d1a049 Compare January 2, 2024 11:02

onur-ozkan approved these changes Jan 2, 2024

View reviewed changes

src/bootstrap/src/core/sanity.rs Outdated Show resolved Hide resolved

saethlin force-pushed the remove-linker-requirement branch from 7d1a049 to 1abf9ea Compare January 2, 2024 21:40

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

RalfJung reviewed Jan 3, 2024

View reviewed changes

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 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

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 added the merged-by-bors This PR was explicitly merged by bors. label Jan 7, 2024

bors merged commit 78c988f into rust-lang:master Jan 7, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 7, 2024

saethlin deleted the remove-linker-requirement branch January 7, 2024 02:34

saethlin mentioned this pull request Jan 21, 2024

Use the same mir-opt bless targets on all platforms #120060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

saethlin commented Dec 17, 2023 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

rustbot commented Jan 1, 2024

saethlin commented Jan 1, 2024

saethlin commented Jan 1, 2024

saethlin commented Jan 2, 2024

onur-ozkan left a comment

onur-ozkan commented Jan 2, 2024

saethlin commented Jan 2, 2024

onur-ozkan commented Jan 2, 2024

saethlin commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

RalfJung Jan 3, 2024

saethlin Jan 3, 2024 •

edited

Loading

RalfJung Jan 3, 2024 •

edited

Loading

saethlin Jan 3, 2024

RalfJung Jan 4, 2024

saethlin Jan 4, 2024 •

edited

Loading

mati865 commented Jan 6, 2024

saethlin commented Jan 6, 2024

bors commented Jan 6, 2024

bors commented Jan 6, 2024

rust-log-analyzer commented Jan 6, 2024

bors commented Jan 6, 2024

saethlin commented Jan 6, 2024

bors commented Jan 7, 2024

bors commented Jan 7, 2024

rust-timer commented Jan 7, 2024

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

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

Conversation

saethlin commented Dec 17, 2023 • edited Loading

This comment has been minimized.

This comment has been minimized.

rustbot commented Jan 1, 2024

saethlin commented Jan 1, 2024

saethlin commented Jan 1, 2024

saethlin commented Jan 2, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan commented Jan 2, 2024

saethlin commented Jan 2, 2024

onur-ozkan commented Jan 2, 2024

saethlin commented Jan 3, 2024

bors commented Jan 3, 2024

bors commented Jan 3, 2024

RalfJung Jan 3, 2024

Choose a reason for hiding this comment

saethlin Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

saethlin Jan 3, 2024

Choose a reason for hiding this comment

RalfJung Jan 4, 2024

Choose a reason for hiding this comment

saethlin Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

mati865 commented Jan 6, 2024

saethlin commented Jan 6, 2024

bors commented Jan 6, 2024

bors commented Jan 6, 2024

rust-log-analyzer commented Jan 6, 2024

bors commented Jan 6, 2024

saethlin commented Jan 6, 2024

bors commented Jan 7, 2024

bors commented Jan 7, 2024

rust-timer commented Jan 7, 2024

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

saethlin commented Dec 17, 2023 •

edited

Loading

saethlin Jan 3, 2024 •

edited

Loading

RalfJung Jan 3, 2024 •

edited

Loading

saethlin Jan 4, 2024 •

edited

Loading