Skip to content

Commit

Permalink
Auto merge of #120060 - saethlin:mir-opt-bless-targets, r=wesleywiser
Browse files Browse the repository at this point in the history 
Use the same mir-opt bless targets on all platforms

This undoes some of the implementation in #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
  • Loading branch information
@bors
bors committed Feb 7, 2024
2 parents 0809f78 + 3c7a8b9 commit d6c46a2
Show file tree
Hide file tree
Showing 16 changed files with 226 additions and 269 deletions.
34 changes: 24 additions & 10 deletions src/bootstrap/src/core/build_steps/compile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ impl Std {
is_for_mir_opt_tests: false,
}
}

fn copy_extra_objects(
&self,
builder: &Builder<'_>,
compiler: &Compiler,
target: TargetSelection,
) -> Vec<(PathBuf, DependencyType)> {
let mut deps = Vec::new();
if !self.is_for_mir_opt_tests {
deps.extend(copy_third_party_objects(builder, &compiler, target));
deps.extend(copy_self_contained_objects(builder, &compiler, target));
}
deps
}
}

impl Step for Std {
Expand Down Expand Up @@ -159,8 +173,7 @@ impl Step for Std {
{
builder.info("WARNING: Using a potentially old libstd. This may not behave well.");

copy_third_party_objects(builder, &compiler, target);
copy_self_contained_objects(builder, &compiler, target);
self.copy_extra_objects(builder, &compiler, target);

builder.ensure(StdLink::from_std(self, compiler));
return;
Expand Down Expand Up @@ -193,15 +206,13 @@ impl Step for Std {

// Even if we're not building std this stage, the new sysroot must
// still contain the third party objects needed by various targets.
copy_third_party_objects(builder, &compiler, target);
copy_self_contained_objects(builder, &compiler, target);
self.copy_extra_objects(builder, &compiler, target);

builder.ensure(StdLink::from_std(self, compiler_to_use));
return;
}

target_deps.extend(copy_third_party_objects(builder, &compiler, target));
target_deps.extend(copy_self_contained_objects(builder, &compiler, target));
target_deps.extend(self.copy_extra_objects(builder, &compiler, target));

// The LLD wrappers and `rust-lld` are self-contained linking components that can be
// necessary to link the stdlib on some targets. We'll also need to copy these binaries to
Expand All @@ -222,10 +233,13 @@ impl Step for Std {
}
}

// We build a sysroot for mir-opt tests using the same trick that Miri does: A check build
// with -Zalways-encode-mir. This frees us from the need to have a target linker, and the
// fact that this is a check build integrates nicely with run_cargo.
let mut cargo = if self.is_for_mir_opt_tests {
let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "rustc");
cargo.arg("-p").arg("std").arg("--crate-type=lib");
std_cargo(builder, target, compiler.stage, &mut cargo);
let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "check");
cargo.rustflag("-Zalways-encode-mir");
cargo.arg("--manifest-path").arg(builder.src.join("library/sysroot/Cargo.toml"));
cargo
} else {
let mut cargo = builder.cargo(compiler, Mode::Std, SourceType::InTree, target, "build");
Expand Down Expand Up @@ -257,7 +271,7 @@ impl Step for Std {
vec![],
&libstd_stamp(builder, compiler, target),
target_deps,
false,
self.is_for_mir_opt_tests, // is_check
false,
);

Expand Down
116 changes: 37 additions & 79 deletions src/bootstrap/src/core/build_steps/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use crate::core::builder::{Builder, Compiler, Kind, RunConfig, ShouldRun, Step};
use crate::core::config::flags::get_completion;
use crate::core::config::flags::Subcommand;
use crate::core::config::TargetSelection;
use crate::utils;
use crate::utils::cache::{Interned, INTERNER};
use crate::utils::exec::BootstrapCommand;
use crate::utils::helpers::{
Expand All @@ -38,23 +37,6 @@ use crate::{envify, CLang, DocTests, GitRepo, Mode};

const ADB_TEST_DIR: &str = "/data/local/tmp/work";

// mir-opt tests have different variants depending on whether a target is 32bit or 64bit, and
// blessing them requires blessing with each target. To aid developers, when blessing the mir-opt
// test suite the corresponding target of the opposite pointer size is also blessed.
//
// This array serves as the known mappings between 32bit and 64bit targets. If you're developing on
// a target where a target with the opposite pointer size exists, feel free to add it here.
const MIR_OPT_BLESS_TARGET_MAPPING: &[(&str, &str)] = &[
// (32bit, 64bit)
("i686-unknown-linux-gnu", "x86_64-unknown-linux-gnu"),
("i686-unknown-linux-musl", "x86_64-unknown-linux-musl"),
("i686-pc-windows-msvc", "x86_64-pc-windows-msvc"),
("i686-pc-windows-gnu", "x86_64-pc-windows-gnu"),
// ARM Macs don't have a corresponding 32-bit target that they can (easily)
// build for, so there is no entry for "aarch64-apple-darwin" here.
// Likewise, i686 for macOS is no longer possible to build.
];

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct CrateBootstrap {
path: Interned<PathBuf>,
Expand Down Expand Up @@ -1487,46 +1469,18 @@ impl Step for MirOpt {
})
};

// We use custom logic to bless the mir-opt suite: mir-opt tests have multiple variants
// (32bit vs 64bit, and panic=abort vs panic=unwind), and all of them needs to be blessed.
// When blessing, we try best-effort to also bless the other variants, to aid developers.
if builder.config.cmd.bless() {
let targets = MIR_OPT_BLESS_TARGET_MAPPING
.iter()
.filter(|(target_32bit, target_64bit)| {
*target_32bit == &*self.target.triple || *target_64bit == &*self.target.triple
})
.next()
.map(|(target_32bit, target_64bit)| {
let target_32bit = TargetSelection::from_user(target_32bit);
let target_64bit = TargetSelection::from_user(target_64bit);

// Running compiletest requires a C compiler to be available, but it might not
// have been detected by bootstrap if the target we're testing wasn't in the
// --target flags.
if !builder.cc.borrow().contains_key(&target_32bit) {
utils::cc_detect::find_target(builder, target_32bit);
}
if !builder.cc.borrow().contains_key(&target_64bit) {
utils::cc_detect::find_target(builder, target_64bit);
}
// All that we really need to do is cover all combinations of 32/64-bit and unwind/abort,
// but while we're at it we might as well flex our cross-compilation support. This
// selection covers all our tier 1 operating systems and architectures using only tier
// 1 targets.

vec![target_32bit, target_64bit]
})
.unwrap_or_else(|| {
eprintln!(
"\
Note that not all variants of mir-opt tests are going to be blessed, as no mapping between
a 32bit and a 64bit target was found for {target}.
You can add that mapping by changing MIR_OPT_BLESS_TARGET_MAPPING in src/bootstrap/test.rs",
target = self.target,
);
vec![self.target]
});

for target in targets {
run(target);
for target in ["aarch64-unknown-linux-gnu", "i686-pc-windows-msvc"] {
run(TargetSelection::from_user(target));
}

for target in ["x86_64-apple-darwin", "i686-unknown-linux-musl"] {
let target = TargetSelection::from_user(target);
let panic_abort_target = builder.ensure(MirOptPanicAbortSyntheticTarget {
compiler: self.compiler,
base: target,
Expand Down Expand Up @@ -1616,27 +1570,27 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
.ensure(dist::DebuggerScripts { sysroot: builder.sysroot(compiler), host: target });
}

if suite == "mir-opt" {
builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
builder.ensure(compile::Std::new(compiler, target));
}
// Also provide `rust_test_helpers` for the host.
builder.ensure(TestHelpers { target: compiler.host });

// ensure that `libproc_macro` is available on the host.
builder.ensure(compile::Std::new(compiler, compiler.host));

// Also provide `rust_test_helpers` for the host.
builder.ensure(TestHelpers { target: compiler.host });

// As well as the target, except for plain wasm32, which can't build it
if suite != "mir-opt" && !target.contains("wasm") && !target.contains("emscripten") {
builder.ensure(TestHelpers { target });
}

builder.ensure(RemoteCopyLibs { compiler, target });

let mut cmd = builder.tool_cmd(Tool::Compiletest);

if suite == "mir-opt" {
builder.ensure(compile::Std::new_for_mir_opt_tests(compiler, target));
} else {
builder.ensure(compile::Std::new(compiler, target));
}

builder.ensure(RemoteCopyLibs { compiler, target });

// compiletest currently has... a lot of arguments, so let's just pass all
// of them!

Expand Down Expand Up @@ -1745,11 +1699,13 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
flags.push(format!("-Cdebuginfo={}", builder.config.rust_debuginfo_level_tests));
flags.extend(builder.config.cmd.rustc_args().iter().map(|s| s.to_string()));

if let Some(linker) = builder.linker(target) {
cmd.arg("--target-linker").arg(linker);
}
if let Some(linker) = builder.linker(compiler.host) {
cmd.arg("--host-linker").arg(linker);
if suite != "mir-opt" {
if let Some(linker) = builder.linker(target) {
cmd.arg("--target-linker").arg(linker);
}
if let Some(linker) = builder.linker(compiler.host) {
cmd.arg("--host-linker").arg(linker);
}
}

let mut hostflags = flags.clone();
Expand Down Expand Up @@ -1936,15 +1892,17 @@ NOTE: if you're sure you want to do this, please open an issue as to why. In the
cmd.arg("--remote-test-client").arg(builder.tool_exe(Tool::RemoteTestClient));
}

// Running a C compiler on MSVC requires a few env vars to be set, to be
// sure to set them here.
//
// Note that if we encounter `PATH` we make sure to append to our own `PATH`
// rather than stomp over it.
if !builder.config.dry_run() && target.is_msvc() {
for &(ref k, ref v) in builder.cc.borrow()[&target].env() {
if k != "PATH" {
cmd.env(k, v);
if suite != "mir-opt" {
// Running a C compiler on MSVC requires a few env vars to be set, to be
// sure to set them here.
//
// Note that if we encounter `PATH` we make sure to append to our own `PATH`
// rather than stomp over it.
if !builder.config.dry_run() && target.is_msvc() {
for &(ref k, ref v) in builder.cc.borrow()[&target].env() {
if k != "PATH" {
cmd.env(k, v);
}
}
}
}
Expand Down
Loading

0 comments on commit d6c46a2

Please sign in to comment.