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

Support when build-std is set for Cargo #3078

Closed
wants to merge 2 commits into from
Closed
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,7 @@ binaries, and as such worth documenting:
perform verbose logging.
* `MIRI_HOST_SYSROOT` is set by bootstrap to tell `cargo-miri` which sysroot to use for *host*
operations.
* `MIRI_STD_AWARE_CARGO` is set by `cargo-miri` when Cargo has `build-std` set to tell the Miri driver not to use the value of `MIRI_SYSROOT` and to inject compiler flags typically used for building the Miri sysroot whenever a crate from the standard library is compiled.

[testing-miri]: CONTRIBUTING.md#testing-the-miri-driver

Expand Down
48 changes: 37 additions & 11 deletions cargo-miri/src/phases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,30 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
let target = get_arg_flag_value("--target");
let target = target.as_ref().unwrap_or(host);

// We always setup.
let miri_sysroot = setup(&subcommand, target, &rustc_version, verbose);
let has_build_std = is_build_std_set();
if has_build_std {
if env::var_os("MIRI_SYSROOT").is_some() {
println!("WARNING: `MIRI_SYSROOT` has no effect when Cargo has `build-std` enabled.");
}
if env::var_os("MIRI_NO_STD").is_some() {
println!("WARNING: `MIRI_NO_STD` has no effect when Cargo has `build-std` enabled.");
}
}
let cargo_cmd = match subcommand {
Copy link
Member

Choose a reason for hiding this comment

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

Why did you move the cargo_cmd logic up? This is confusing now, since the match has too many jobs.

Also we want to call setup before any of the other logic here. I think this should be more like

let miri_sysroot = if has_build_std {
  Some(setup(...))
} else { None };

MiriCommand::Forward(ref s) => s,
MiriCommand::Setup => {
if has_build_std {
println!(
"NOTE: `cargo miri setup` is unnecessary when Cargo has `build-std` enabled. \
libstd and friends will be built on-the-fly when `cargo miri run` or `cargo \
miri test` is invoked."
);
} else {
setup(&subcommand, target, &rustc_version, verbose);
}
return; // `cargo miri setup` stops here.
}
};

// Invoke actual cargo for the job, but with different flags.
// We re-use `cargo test` and `cargo run`, which makes target and binary handling very easy but
Expand All @@ -107,13 +129,9 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
.into_os_string()
.into_string()
.expect("current executable path is not valid UTF-8");
let cargo_cmd = match subcommand {
MiriCommand::Forward(s) => s,
MiriCommand::Setup => return, // `cargo miri setup` stops here.
};
let metadata = get_cargo_metadata();
let mut cmd = cargo();
cmd.arg(&cargo_cmd);
cmd.arg(cargo_cmd);
// In nextest we have to also forward the main `verb`.
if cargo_cmd == "nextest" {
cmd.arg(
Expand Down Expand Up @@ -159,8 +177,12 @@ pub fn phase_cargo_miri(mut args: impl Iterator<Item = String>) {
// Forward all further arguments (not consumed by `ArgSplitFlagValue`) to cargo.
cmd.args(args);

// Let it know where the Miri sysroot lives.
cmd.env("MIRI_SYSROOT", miri_sysroot);
if has_build_std {
cmd.env("MIRI_STD_AWARE_CARGO", "1");
} else {
// Let it know where the Miri sysroot lives.
cmd.env("MIRI_SYSROOT", setup(&subcommand, target, &rustc_version, verbose));
}
// Set `RUSTC_WRAPPER` to ourselves. Cargo will prepend that binary to its usual invocation,
// i.e., the first argument is `rustc` -- which is what we use in `main` to distinguish
// the two codepaths. (That extra argument is why we prefer this over setting `RUSTC`.)
Expand Down Expand Up @@ -606,8 +628,12 @@ pub fn phase_rustdoc(mut args: impl Iterator<Item = String>) {
// which are disabled by default. We first need to enable them explicitly:
cmd.arg("-Z").arg("unstable-options");

// rustdoc needs to know the right sysroot.
cmd.arg("--sysroot").arg(env::var_os("MIRI_SYSROOT").unwrap());
if let Some(sysroot) = env::var_os("MIRI_SYSROOT") {
// rustdoc needs to know the right sysroot.
cmd.arg("--sysroot").arg(sysroot);
} else if env::var_os("MIRI_STD_AWARE_CARGO").is_none() {
show_error!("`MIRI_SYSROOT` must be set unless `MIRI_STD_AWARE_CARGO` is set");
};
// make sure the 'miri' flag is set for rustdoc
cmd.arg("--cfg").arg("miri");

Expand Down
17 changes: 17 additions & 0 deletions cargo-miri/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,20 @@ pub fn debug_cmd(prefix: &str, verbose: usize, cmd: &Command) {
}
eprintln!("{prefix} running command: {cmd:?}");
}

/// Checks if Cargo's build-std setting is enabled for one or more crates.
pub fn is_build_std_set() -> bool {
let cmd = cargo()
Copy link
Member

Choose a reason for hiding this comment

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

You need to pass cargo_extra_flags() as arguments to ensure that if the user sets --cfg unstable.build-std that will be respected. Also what about -Zbuild-std? I don't think I understand how these flags even interact.

It might be worth having a little helper function that invokes cargo config to get the value of an arbitrary key.

// TODO: remove the -Z argument when this subcommand is stabilized;
// see <https://github.com/rust-lang/cargo/issues/9301>
.args(&["-Zunstable-options", "config", "get", "--format=json-value", "unstable.build-std"])
.output()
.expect("failed to get Cargo config");
// TODO: Cargo will exit with an error code if the `unstable.build-std` key is not set. Should
// we still return false if an unrelated error has occurred?
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd distinguish these cases. Which status code does cargo return when the key is absent? The least we can do is raise an error on any unexpected status code.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I think Cargo returns 101 for most errors (unless I am misunderstanding your question). This includes when the key is absent but also, e.g., when the key a.b cannot be resolved in the first place because a exists and is not a table. I'm sure there are many other error cases for cargo config get which return 101.

Relevant Cargo code: https://github.com/rust-lang/cargo/blob/f5418fcfebe7d01536b891c8b2ceb32c4eaf098c/src/cargo/util/errors.rs#L324C1-L341C2

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that's unfortunate.

But still you can then treat only error 101 as "absent" and all other codes as actual errors.

if !cmd.status.success() {
return false;
}

cmd.stdout.get(0..2) == Some(b"[\"")
}
75 changes: 61 additions & 14 deletions src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,21 +242,49 @@ fn run_compiler(
callbacks: &mut (dyn rustc_driver::Callbacks + Send),
) -> ! {
if target_crate {
// Miri needs a custom sysroot for target crates.
// If no `--sysroot` is given, the `MIRI_SYSROOT` env var is consulted to find where
// that sysroot lives, and that is passed to rustc.
let sysroot_flag = "--sysroot";
if !args.iter().any(|e| e == sysroot_flag) {
// Using the built-in default here would be plain wrong, so we *require*
// the env var to make sure things make sense.
let miri_sysroot = env::var("MIRI_SYSROOT").unwrap_or_else(|_| {
show_error!(
"Miri was invoked in 'target' mode without `MIRI_SYSROOT` or `--sysroot` being set"
)
});
if env::var_os("MIRI_STD_AWARE_CARGO").is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

I would strongly prefer if we could avoid having such hacks in the Miri driver. This should all be taken care of by cargo-miri.

cargo-miri already injects itself into every invocation from cargo, so it should be entirely possible to do any further command-line argument adjustments there rather than doing them inside the Miri driver.

let crate_name = parse_crate_name(&args).unwrap_or_else(|| {
show_error!("`MIRI_STD_AWARE_CARGO` is set but `--crate-name` is not")
})
.to_owned();
// This list should be kept in sync with the one from
// `cargo/core/compiler/standard_lib.rs`.
if matches!(
crate_name.as_str(),
"std"
| "core"
| "alloc"
| "proc_macro"
| "panic_abort"
| "panic_unwind"
| "test"
| "compiler_builtins"
) {
// We are compiling the standard library.
args.push("-Cdebug-assertions=off".into());
Copy link
Author

Choose a reason for hiding this comment

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

I have also written a version of this which attempts to fix existing "-Cdebug-assertions", "-Coverflow-checks", etc. options if they are present (and also accounting for, e.g., if "-C" and "debug-assertions" are separate arguments), and only resorts to appending new arguments if the options are not present. However, I'm not sure if this extra complexity is warranted, because I think rustc can handle duplicate codegen options just fine, so probably the only benefit would be to make the rustc invocation "nicer" for someone debugging the interaction between miri and rustc

Copy link
Member

Choose a reason for hiding this comment

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

Why do we want this? If the user has enabled debug assertions then we should honor that. We should behave like regular cargo run, which for build-std will also apply flags to the std build.

Miri no longer sets -Cdebug-assertions=on either, so I see no reason why we'd start messing with -C flags again.

args.push("-Coverflow-checks=on".into());
}
// See `cargo_miri::phase_rustc`.
if crate_name == "panic_abort" {
args.push("-Cpanic=abort".into());
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed? If build-std with regular rustc doesn't need it then we shouldn't need it either. AFAIK build-std will simply only build the panic runtime that is actually needed for this crate and this avoids the issue that comes up when building the panic runtime for a sysroot.

} else {
// Miri needs a custom sysroot for target crates.
// If no `--sysroot` is given, the `MIRI_SYSROOT` env var is consulted to find where
// that sysroot lives, and that is passed to rustc.
let sysroot_flag = "--sysroot";
if !args.iter().any(|e| e == sysroot_flag) {
// Using the built-in default here would be plain wrong, so we *require*
// the env var to make sure things make sense.
let miri_sysroot = env::var("MIRI_SYSROOT").unwrap_or_else(|_| {
show_error!(
"Miri was invoked in 'target' mode without `MIRI_SYSROOT` or `--sysroot` being set"
)
});

args.push(sysroot_flag.to_owned());
args.push(miri_sysroot);
args.push(sysroot_flag.to_owned());
args.push(miri_sysroot);
}
}
}

Expand Down Expand Up @@ -284,6 +312,25 @@ fn parse_comma_list<T: FromStr>(input: &str) -> Result<Vec<T>, T::Err> {
input.split(',').map(str::parse::<T>).collect()
}

/// Extracts the value associated with the `--crate-name` argument from the given command line.
fn parse_crate_name(args: &Vec<String>) -> Option<&str> {
let mut iter = args.iter();
while let Some(e) = iter.next() {
let Some(("", tail)) = e.split_once("--crate-name") else {
continue;
};
if let Some(("", val)) = tail.split_once("=") {
return Some(val);
} else if tail == "" {
return Some(iter.next().as_ref().unwrap_or_else(|| {
show_error!("--crate-name is missing required argument")
}));
}
show_error!("--crate-name argument is invalid");
}
None
}

fn main() {
let handler = EarlyErrorHandler::new(ErrorOutputType::default());

Expand Down
1 change: 1 addition & 0 deletions test-cargo-miri/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ issue_1567 = { path = "issue-1567" }
issue_1691 = { path = "issue-1691" }
issue_1705 = { path = "issue-1705" }
issue_1760 = { path = "issue-1760" }
issue_2705 = { path = "issue-2705" }
Copy link
Member

Choose a reason for hiding this comment

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

I was expecting we would add something somewhere that calls cargo miri run -Zbuild-std; why is this a separate crate?

issue_rust_86261 = { path = "issue-rust-86261" }

[dev-dependencies]
Expand Down
5 changes: 5 additions & 0 deletions test-cargo-miri/issue-2705/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[build]
target = "ee.json"

[unstable]
build-std = ["core"]
5 changes: 5 additions & 0 deletions test-cargo-miri/issue-2705/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[package]
name = "issue_2705"
version = "0.1.0"
authors = ["Miri Team"]
edition = "2018"
18 changes: 18 additions & 0 deletions test-cargo-miri/issue-2705/ee.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
"arch": "mips",
"cpu": "mips2",
"data-layout": "e-m:m-p:32:32-i8:8:32-i16:16:32-i64:64-n32-S64",
"features": "+mips2,+soft-float",
"linker-flavor": "ld.lld",
"llvm-abiname": "o32",
"llvm-target": "mipsel-none-elf",
"max-atomic-width": 32,
"os": "none",
"panic-strategy": "abort",
"position-independent-executables": false,
"relro-level": "full",
"target-c-int-width": "32",
"target-endian": "little",
"target-pointer-width": "32",
"relocation-model": "static"
}
5 changes: 5 additions & 0 deletions test-cargo-miri/issue-2705/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
use byteorder::{ByteOrder, LittleEndian};

pub fn use_the_dependency() {
let _n = <LittleEndian as ByteOrder>::read_u32(&[1, 2, 3, 4]);
}
Loading