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

Refactor the way bootstrap invokes cargo miri #123192

Merged
merged 10 commits into from Apr 1, 2024
2 changes: 1 addition & 1 deletion src/bootstrap/src/bin/rustc.rs
Expand Up @@ -150,7 +150,7 @@ fn main() {
{
cmd.arg("-Ztls-model=initial-exec");
}
} else if std::env::var("MIRI").is_err() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was added here but at least in local testing is not needed any more. Let's see what CI says.

Cc @Urgau

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not needed anymore, that probably mean that Miri is respecting the RUSTFLAGS set by bootstrap (or always passing a target). It's probably not worth checking if there are any --cfg bootstrap or --check-cfg args being passed, since we will quickly see if CI fails.

Good to see this hack disappear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Miri always passes a target, but that has always been the case. Nothing recently changed wrt RUSTFLAGS either.

But maybe somehow this interacts with the other changes in this PR.

} else {
// Find any host flags that were passed by bootstrap.
// The flags are stored in a RUSTC_HOST_FLAGS variable, separated by spaces.
if let Ok(flags) = std::env::var("RUSTC_HOST_FLAGS") {
Expand Down
6 changes: 0 additions & 6 deletions src/bootstrap/src/core/build_steps/compile.rs
Expand Up @@ -1130,12 +1130,6 @@ pub fn rustc_cargo_env(
cargo.env("CFG_DEFAULT_LINKER", s);
}

if builder.config.rustc_parallel {
// keep in sync with `bootstrap/lib.rs:Build::rustc_features`
// `cfg` option for rustc, `features` option for cargo, for conditional compilation
cargo.rustflag("--cfg=parallel_compiler");
cargo.rustdocflag("--cfg=parallel_compiler");
}
Copy link
Member Author

@RalfJung RalfJung Mar 30, 2024

Choose a reason for hiding this comment

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

I have traced back every caller of this function and verified that they all call prepare_tool_cargo with one of the modes that triggers the flag.

if builder.config.rust_verify_llvm_ir {
cargo.env("RUSTC_VERIFY_LLVM_IR", "1");
}
Expand Down
34 changes: 19 additions & 15 deletions src/bootstrap/src/core/build_steps/run.rs
Expand Up @@ -121,8 +121,6 @@ impl Step for ReplaceVersionPlaceholder {

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Miri {
stage: u32,
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized this field is unnecessary. At least it seems unnecessary, I don't fully understand how this make_run vs run stuff is meant to be used.

host: TargetSelection,
target: TargetSelection,
}

Expand All @@ -135,29 +133,35 @@ impl Step for Miri {
}

fn make_run(run: RunConfig<'_>) {
run.builder.ensure(Miri {
stage: run.builder.top_stage,
host: run.build_triple(),
Copy link
Member Author

Choose a reason for hiding this comment

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

run.build_triple() returns builder.cmd.build; I assume that's always the same as builder.build.build?

Copy link
Member

Choose a reason for hiding this comment

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

run.build_triple() returns builder.cmd.build

run.build_triple() returns builder.build.build already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, you are right... and builder.cmd.build does not exist.
But some places use builder.config.build. That is hopefully the same as builder.build.build.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, they are identical.

target: run.target,
});
run.builder.ensure(Miri { target: run.target });
}

fn run(self, builder: &Builder<'_>) {
let stage = self.stage;
let host = self.host;
let host = builder.build.build;
let target = self.target;
let compiler = builder.compiler(stage, host);

let miri =
builder.ensure(tool::Miri { compiler, target: self.host, extra_features: Vec::new() });
let miri_sysroot = test::Miri::build_miri_sysroot(builder, compiler, &miri, target);
let stage = builder.top_stage;
if stage == 0 {
eprintln!("miri cannot be run at stage 0");
std::process::exit(1);
}

// This compiler runs on the host, we'll just use it for the target.
let target_compiler = builder.compiler(stage, host);
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want. Rustdoc should be linked in the same way as the
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let host_compiler = builder.compiler(stage - 1, host);
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

// Get a target sysroot for Miri.
let miri_sysroot = test::Miri::build_miri_sysroot(builder, target_compiler, target);

// # Run miri.
// Running it via `cargo run` as that figures out the right dylib path.
// add_rustc_lib_path does not add the path that contains librustc_driver-<...>.so.
let mut miri = tool::prepare_tool_cargo(
builder,
compiler,
host_compiler,
Mode::ToolRustc,
host,
"run",
Expand Down
156 changes: 75 additions & 81 deletions src/bootstrap/src/core/build_steps/test.rs
Expand Up @@ -493,8 +493,6 @@ impl Step for RustDemangler {

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Miri {
stage: u32,
host: TargetSelection,
target: TargetSelection,
}

Expand All @@ -503,41 +501,26 @@ impl Miri {
pub fn build_miri_sysroot(
builder: &Builder<'_>,
compiler: Compiler,
miri: &Path,
target: TargetSelection,
) -> String {
let miri_sysroot = builder.out.join(compiler.host.triple).join("miri-sysroot");
let mut cargo = tool::prepare_tool_cargo(
let mut cargo = builder::Cargo::new(
builder,
compiler,
Mode::ToolRustc,
compiler.host,
"run",
"src/tools/miri/cargo-miri",
SourceType::InTree,
&[],
Mode::Std,
SourceType::Submodule,
target,
"miri-setup",
);
cargo.add_rustc_lib_path(builder);
cargo.arg("--").arg("miri").arg("setup");
cargo.arg("--target").arg(target.rustc_target_arg());

// Tell `cargo miri setup` where to find the sources.
cargo.env("MIRI_LIB_SRC", builder.src.join("library"));
// Tell it where to find Miri.
cargo.env("MIRI", miri);
// Tell it where to put the sysroot.
cargo.env("MIRI_SYSROOT", &miri_sysroot);
// Debug things.
cargo.env("RUST_BACKTRACE", "1");

let mut cargo = Command::from(cargo);
let _guard = builder.msg(
Kind::Build,
compiler.stage + 1,
"miri sysroot",
compiler.host,
compiler.host,
);
let _guard =
builder.msg(Kind::Build, compiler.stage, "miri sysroot", compiler.host, target);
builder.run(&mut cargo);

// # Determine where Miri put its sysroot.
Expand Down Expand Up @@ -574,68 +557,75 @@ impl Step for Miri {
}

fn make_run(run: RunConfig<'_>) {
run.builder.ensure(Miri {
stage: run.builder.top_stage,
host: run.build_triple(),
target: run.target,
});
run.builder.ensure(Miri { target: run.target });
}

/// Runs `cargo test` for miri.
fn run(self, builder: &Builder<'_>) {
let stage = self.stage;
let host = self.host;
let host = builder.build.build;
let target = self.target;
let compiler = builder.compiler(stage, host);
// We need the stdlib for the *next* stage, as it was built with this compiler that also built Miri.
// Except if we are at stage 2, the bootstrap loop is complete and we can stick with our current stage.
let compiler_std = builder.compiler(if stage < 2 { stage + 1 } else { stage }, host);

let miri =
builder.ensure(tool::Miri { compiler, target: self.host, extra_features: Vec::new() });
let _cargo_miri = builder.ensure(tool::CargoMiri {
compiler,
target: self.host,
let stage = builder.top_stage;
if stage == 0 {
eprintln!("miri cannot be tested at stage 0");
std::process::exit(1);
}

// This compiler runs on the host, we'll just use it for the target.
let target_compiler = builder.compiler(stage, host);
// Similar to `compile::Assemble`, build with the previous stage's compiler. Otherwise
// we'd have stageN/bin/rustc and stageN/bin/rustdoc be effectively different stage
// compilers, which isn't what we want. Rustdoc should be linked in the same way as the
// rustc compiler it's paired with, so it must be built with the previous stage compiler.
let host_compiler = builder.compiler(stage - 1, host);
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized this is still not quite consistent with rustdoc. In rustdoc the stage adjustment is done in the tool step, not the test step. But the tool steps are macro-generated for a bunch of tools and I don't want to do a change that could break clippy or so.^^

And arguably rustdoc is the weird one here; ./x.py build compiler/rustc --stage 0 and ./x.py build miri --stage 0 should be consistent (and they are with this PR); ./x.py build rustdoc --stage 0 is a NOP that does nothing.


// Build our tools.
let miri = builder.ensure(tool::Miri {
compiler: host_compiler,
target: host,
extra_features: Vec::new(),
});
// the ui tests also assume cargo-miri has been built
builder.ensure(tool::CargoMiri {
compiler: host_compiler,
target: host,
extra_features: Vec::new(),
});
// The stdlib we need might be at a different stage. And just asking for the
// sysroot does not seem to populate it, so we do that first.
builder.ensure(compile::Std::new(compiler_std, host));
let sysroot = builder.sysroot(compiler_std);
// We also need a Miri sysroot.
let miri_sysroot = Miri::build_miri_sysroot(builder, compiler, &miri, target);

// We also need sysroots, for Miri and for the host (the latter for build scripts).
// This is for the tests so everything is done with the target compiler.
let miri_sysroot = Miri::build_miri_sysroot(builder, target_compiler, target);
builder.ensure(compile::Std::new(target_compiler, host));
let sysroot = builder.sysroot(target_compiler);

// # Run `cargo test`.
// This is with the Miri crate, so it uses the host compiler.
let mut cargo = tool::prepare_tool_cargo(
builder,
compiler,
host_compiler,
Mode::ToolRustc,
host,
"test",
"src/tools/miri",
SourceType::InTree,
&[],
);
let _guard = builder.msg_sysroot_tool(Kind::Test, compiler.stage, "miri", host, target);

cargo.add_rustc_lib_path(builder);

// We can NOT use `run_cargo_test` since Miri's integration tests do not use the usual test
// harness and therefore do not understand the flags added by `add_flags_and_try_run_test`.
let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", host_compiler, host, builder);

// miri tests need to know about the stage sysroot
cargo.env("MIRI_SYSROOT", &miri_sysroot);
cargo.env("MIRI_HOST_SYSROOT", &sysroot);
cargo.env("MIRI", &miri);
if builder.config.locked_deps {
// enforce lockfiles
cargo.env("CARGO_EXTRA_FLAGS", "--locked");
}

// Set the target.
cargo.env("MIRI_TEST_TARGET", target.rustc_target_arg());

// This can NOT be `run_cargo_test` since the Miri test runner
// does not understand the flags added by `add_flags_and_try_run_test`.
let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", compiler, target, builder);
{
let _guard = builder.msg_sysroot_tool(Kind::Test, stage, "miri", host, target);
let _time = helpers::timeit(builder);
builder.run(&mut cargo);
}
Expand All @@ -650,8 +640,14 @@ impl Step for Miri {
// Optimizations can change error locations and remove UB so don't run `fail` tests.
cargo.args(["tests/pass", "tests/panic"]);

let mut cargo = prepare_cargo_test(cargo, &[], &[], "miri", compiler, target, builder);
{
let _guard = builder.msg_sysroot_tool(
Kind::Test,
stage,
"miri (mir-opt-level 4)",
host,
target,
);
let _time = helpers::timeit(builder);
builder.run(&mut cargo);
}
Expand All @@ -660,42 +656,40 @@ impl Step for Miri {
// # Run `cargo miri test`.
// This is just a smoke test (Miri's own CI invokes this in a bunch of different ways and ensures
// that we get the desired output), but that is sufficient to make sure that the libtest harness
// itself executes properly under Miri.
// itself executes properly under Miri, and that all the logic in `cargo-miri` does not explode.
// This is running the build `cargo-miri` for the given target, so we need the target compiler.
let mut cargo = tool::prepare_tool_cargo(
builder,
compiler,
Mode::ToolRustc,
host,
"run",
"src/tools/miri/cargo-miri",
target_compiler,
Mode::ToolStd, // it's unclear what to use here, we're not building anything just doing a smoke test!
Copy link
Member

Choose a reason for hiding this comment

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

ToolBootstrap is the right choice if you don't build anything.

ref:

match self.mode {
Mode::ToolRustc => {
builder.ensure(compile::Std::new(compiler, compiler.host));
builder.ensure(compile::Rustc::new(compiler, target));
}
Mode::ToolStd => builder.ensure(compile::Std::new(compiler, target)),
Mode::ToolBootstrap => {} // uses downloaded stage0 compiler libs
_ => panic!("unexpected Mode for tool build"),
}

Copy link
Member Author

@RalfJung RalfJung Mar 30, 2024

Choose a reason for hiding this comment

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

That doesn't work for stages > 0 though, because of this assertion:

let use_snapshot = mode == Mode::ToolBootstrap;
assert!(!use_snapshot || stage == 0 || self.local_rebuild);

target,
"miri-test",
"src/tools/miri/test-cargo-miri",
SourceType::Submodule,
&[],
);
cargo.add_rustc_lib_path(builder);
cargo.arg("--").arg("miri").arg("test");
if builder.config.locked_deps {
cargo.arg("--locked");
}
cargo
.arg("--manifest-path")
.arg(builder.src.join("src/tools/miri/test-cargo-miri/Cargo.toml"));
cargo.arg("--target").arg(target.rustc_target_arg());
cargo.arg("--").args(builder.config.test_args());

// `prepare_tool_cargo` sets RUSTDOC to the bootstrap wrapper and RUSTDOC_REAL to a dummy path as this is a "run", not a "test".
// Also, we want the rustdoc from the "next" stage for the same reason that we build a std from the next stage.
// So let's just set that here, and bypass bootstrap's RUSTDOC (just like cargo-miri already ignores bootstrap's RUSTC_WRAPPER).
cargo.env("RUSTDOC", builder.rustdoc(compiler_std));
// We're not using `prepare_cargo_test` so we have to do this ourselves.
// (We're not using that as the test-cargo-miri crate is not known to bootstrap.)
match builder.doc_tests {
DocTests::Yes => {}
DocTests::No => {
cargo.args(["--lib", "--bins", "--examples", "--tests", "--benches"]);
}
DocTests::Only => {
cargo.arg("--doc");
}
}

// Tell `cargo miri` where to find things.
// Tell `cargo miri` where to find the sysroots.
cargo.env("MIRI_SYSROOT", &miri_sysroot);
cargo.env("MIRI_HOST_SYSROOT", sysroot);
cargo.env("MIRI", &miri);
// Debug things.
cargo.env("RUST_BACKTRACE", "1");

// Finally, pass test-args and run everything.
cargo.arg("--").args(builder.config.test_args());
let mut cargo = Command::from(cargo);
{
let _guard = builder.msg_sysroot_tool(Kind::Test, stage, "cargo-miri", host, target);
let _time = helpers::timeit(builder);
builder.run(&mut cargo);
}
Expand Down
12 changes: 2 additions & 10 deletions src/bootstrap/src/core/build_steps/tool.rs
Expand Up @@ -469,7 +469,7 @@ impl Step for Rustdoc {
features.push("jemalloc".to_string());
}

let mut cargo = prepare_tool_cargo(
let cargo = prepare_tool_cargo(
builder,
build_compiler,
Mode::ToolRustc,
Expand All @@ -480,10 +480,6 @@ impl Step for Rustdoc {
features.as_slice(),
);

if builder.config.rustc_parallel {
cargo.rustflag("--cfg=parallel_compiler");
}

let _guard = builder.msg_tool(
Mode::ToolRustc,
"rustdoc",
Expand Down Expand Up @@ -732,7 +728,7 @@ impl Step for LlvmBitcodeLinker {
builder.ensure(compile::Std::new(self.compiler, self.compiler.host));
builder.ensure(compile::Rustc::new(self.compiler, self.target));

let mut cargo = prepare_tool_cargo(
let cargo = prepare_tool_cargo(
builder,
self.compiler,
Mode::ToolRustc,
Expand All @@ -743,10 +739,6 @@ impl Step for LlvmBitcodeLinker {
&self.extra_features,
);

if builder.config.rustc_parallel {
cargo.rustflag("--cfg=parallel_compiler");
}

builder.run(&mut cargo.into());

let tool_out = builder
Expand Down