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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions src/bootstrap/src/core/build_steps/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ pub struct Std {
/// but we need to use the downloaded copy of std for linking to rustdoc. Allow this to be overriden by `builder.ensure` from other steps.
force_recompile: bool,
extra_rust_args: &'static [&'static str],
is_for_mir_opt_tests: bool,
}

impl Std {
Expand All @@ -56,6 +57,7 @@ impl Std {
crates: Default::default(),
force_recompile: false,
extra_rust_args: &[],
is_for_mir_opt_tests: false,
}
}

Expand All @@ -66,6 +68,18 @@ impl Std {
crates: Default::default(),
force_recompile: true,
extra_rust_args: &[],
is_for_mir_opt_tests: false,
}
}

pub fn new_for_mir_opt_tests(compiler: Compiler, target: TargetSelection) -> Self {
Self {
target,
compiler,
crates: Default::default(),
force_recompile: false,
extra_rust_args: &[],
is_for_mir_opt_tests: true,
}
}

Expand All @@ -80,6 +94,7 @@ impl Std {
crates: Default::default(),
force_recompile: false,
extra_rust_args,
is_for_mir_opt_tests: false,
}
}
}
Expand Down Expand Up @@ -109,6 +124,7 @@ impl Step for Std {
crates,
force_recompile: false,
extra_rust_args: &[],
is_for_mir_opt_tests: false,
});
}

Expand Down Expand Up @@ -206,11 +222,19 @@ impl Step for Std {
}
}

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 {
cargo.arg("-p").arg(krate);
}
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);
cargo
} 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.

cargo.arg("-p").arg(krate);
}
cargo
};

// See src/bootstrap/synthetic_targets.rs
if target.is_synthetic() {
Expand Down
9 changes: 7 additions & 2 deletions src/bootstrap/src/core/build_steps/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1611,15 +1611,20 @@ 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 });
}

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

// 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 !target.contains("wasm") || target.contains("emscripten") {
if suite != "mir-opt" && !target.contains("wasm") && !target.contains("emscripten") {
builder.ensure(TestHelpers { target });
}

Expand Down
8 changes: 7 additions & 1 deletion src/bootstrap/src/core/sanity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,15 @@ impl Finder {
}

pub fn check(build: &mut Build) {
let skip_target_sanity =
let mut skip_target_sanity =
env::var_os("BOOTSTRAP_SKIP_TARGET_SANITY").is_some_and(|s| s == "1" || s == "true");

// Skip target sanity checks when we are doing anything with mir-opt tests or Miri
let skipped_paths = [OsStr::new("mir-opt"), OsStr::new("miri")];
skip_target_sanity |= build.config.paths.iter().any(|path| {
path.components().any(|component| skipped_paths.contains(&component.as_os_str()))
});

let path = env::var_os("PATH").unwrap_or_default();
// On Windows, quotes are invalid characters for filename paths, and if
// one is present as part of the PATH then that can lead to the system
Expand Down
2 changes: 0 additions & 2 deletions src/ci/docker/host-x86_64/x86_64-gnu-tools/checktools.sh
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ else
fi
# We natively run this script on x86_64-unknown-linux-gnu and x86_64-pc-windows-msvc.
# Also cover some other targets via cross-testing, in particular all tier 1 targets.
export BOOTSTRAP_SKIP_TARGET_SANITY=1 # we don't need `cc` for these targets
case $HOST_TARGET in
x86_64-unknown-linux-gnu)
# Only this branch runs in PR CI.
Expand All @@ -62,4 +61,3 @@ case $HOST_TARGET in
exit 1
;;
esac
unset BOOTSTRAP_SKIP_TARGET_SANITY
3 changes: 0 additions & 3 deletions tests/mir-opt/remove_never_const.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
// consts in codegen. We also have tests for this that catches the error, see
// tests/ui/consts/const-eval/index-out-of-bounds-never-type.rs.

// Force generation of optimized mir for functions that do not reach codegen.
// compile-flags: --emit mir,link

#![feature(never_type)]

struct PrintName<T>(T);
Expand Down
Loading