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

Rustup for panic changes #1048

Closed
wants to merge 11 commits into from
2 changes: 1 addition & 1 deletion rust-version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
56237d75b4271a8a2e0f47d86ea76ebf6d966152
a333eed7fc0c903df9d6befcfb40af02148bf255
117 changes: 86 additions & 31 deletions src/bin/cargo-miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::path::{PathBuf, Path};
use std::process::Command;
use std::ops::Not;

const XARGO_MIN_VERSION: (u32, u32, u32) = (0, 3, 17);
const XARGO_MIN_VERSION: (u32, u32, u32) = (0, 3, 18);

const CARGO_MIRI_HELP: &str = r#"Interprets bin crates and tests in Miri

Expand Down Expand Up @@ -80,7 +80,36 @@ fn get_arg_flag_value(name: &str) -> Option<String> {
}
}

fn list_targets() -> impl Iterator<Item=cargo_metadata::Target> {
fn is_build_dep(mut args: impl Iterator<Item = String>) -> bool {
args.any(|arg| arg.starts_with("--emit=") && arg.contains("link"))
}

// Returns whether or not Cargo invoked the wrapper (this binary) to compile
// the final, target crate (either a test for 'cargo test', or a binary for 'cargo run')
// Right now, this is an awful hack that checks several different pieces of information
// to try to figure out if the crate being compiled is the right one.
// Ideally, Cargo would set en environment variable indicating whether or
// not the wrapper is being invoked on the target crate.
// For now, this is the best we can do
fn is_target_crate(is_build_script: bool) -> bool {
// Cargo sets this to the directory containing the manifest of the crate
// the wrapper is being invoekd to compile. This should be unique
// across the entire build (except for build scripts, which we handle below).
// We cannot check the crate name, since this may not be unique
// (e.g. if the build contains multiple versions of the same crate,
// or the same crate from multiple sources)
let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").ok();

// The manifest directory for our target crate. This is set by `cargo-miri`
// (the original invoation by the user) by using `cargo_metadata` to locate
// the manifest.
let expected_dir = std::env::var("MIRI_MAGIC_DIR").expect("MIRI_MAGIC_DIR not set!");

manifest_dir == Some(expected_dir) && !is_build_script

}

fn read_cargo_metadata() -> (impl Iterator<Item=cargo_metadata::Target>, String) {
// We need to get the manifest, and then the metadata, to enumerate targets.
let manifest_path = get_arg_flag_value("--manifest-path").map(|m|
Path::new(&m).canonicalize().unwrap()
Expand Down Expand Up @@ -119,7 +148,7 @@ fn list_targets() -> impl Iterator<Item=cargo_metadata::Target> {
let package = metadata.packages.remove(package_index);

// Finally we got the list of targets to build
package.targets.into_iter()
(package.targets.into_iter(), metadata.workspace_root.to_string_lossy().to_string())
}

/// Returns the path to the `miri` binary
Expand Down Expand Up @@ -266,7 +295,7 @@ fn setup(ask_user: bool) {
show_error(format!("Your xargo is too old; please upgrade to the latest version"))
}
let mut cmd = cargo();
cmd.args(&["install", "xargo", "-f"]);
cmd.args(&["install", "-f", "--git", "https://github.com/Aaron1011/xargo", "--branch", "feature/cargo-mode"]);
ask_to_run(cmd, ask_user, "install a recent enough xargo");
}

Expand Down Expand Up @@ -294,6 +323,7 @@ fn setup(ask_user: bool) {
// The interesting bit: Xargo.toml
File::create(dir.join("Xargo.toml")).unwrap()
.write_all(br#"
cargo_mode = "check"
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
[dependencies.std]
default_features = false
# We need the `panic_unwind` feature because we use the `unwind` panic strategy.
Expand All @@ -318,7 +348,12 @@ path = "lib.rs"
let target = get_arg_flag_value("--target");
let print_sysroot = !ask_user && has_arg_flag("--print-sysroot"); // whether we just print the sysroot path
let mut command = xargo();
command.arg("build").arg("-q");
// This may seen somewhat suprising - we are 'building' libstd
// by running (the equivalent of) `cargo check`. It turns out
// that `cargo check` has exactly the behavior that we want:
// it emits crate metadata (including MIR) without running any
// codegen.
command.arg("check").arg("-q");
Copy link
Member

Choose a reason for hiding this comment

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

The check vs build here just affects the empty "fake crate" that we have for xargo, right? This comment here should be attached to the cargo_mode line above, not the command invocation down here.

command.current_dir(&dir);
command.env("RUSTFLAGS", miri::miri_default_args().join(" "));
command.env("XARGO_HOME", dir.to_str().unwrap());
Expand All @@ -336,6 +371,7 @@ path = "lib.rs"
show_error(format!("Failed to run xargo"));
}


// That should be it! But we need to figure out where xargo built stuff.
// Unfortunately, it puts things into a different directory when the
// architecture matches the host.
Expand Down Expand Up @@ -404,8 +440,10 @@ fn in_cargo_miri() {
return;
}

let (targets, root_dir) = read_cargo_metadata();

// Now run the command.
for target in list_targets() {
for target in targets {
let mut args = std::env::args().skip(skip);
let kind = target.kind.get(0).expect(
"badly formatted cargo metadata: target::kind is an empty array",
Expand All @@ -414,7 +452,7 @@ fn in_cargo_miri() {
// change to add additional arguments. `FLAGS` is set to identify
// this target. The user gets to control what gets actually passed to Miri.
let mut cmd = cargo();
cmd.arg("rustc");
cmd.arg("check");
match (subcommand, kind.as_str()) {
(MiriCommand::Run, "bin") => {
// FIXME: we just run all the binaries here.
Expand All @@ -441,14 +479,17 @@ fn in_cargo_miri() {
}
cmd.arg(arg);
}
// Add `--` (to end the `cargo` flags), and then the user flags. We add markers around the
// user flags to be able to identify them later. "cargo rustc" adds more stuff after this,
// so we have to mark both the beginning and the end.
cmd
.arg("--")
.arg("cargo-miri-marker-begin")
.args(args)
.arg("cargo-miri-marker-end");

let mut prefixed_args = String::new();
for arg in args {
prefixed_args += &arg.len().to_string();
prefixed_args.push(';');
prefixed_args += &arg;
}
Copy link
Member

Choose a reason for hiding this comment

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

When inventing a serialization scheme, at the very least please put this into separate functions.

Though maybe at this point we should just use serde and serialize this as JSON, or so?


cmd.env("MIRI_MAGIC_ARGS", prefixed_args);
cmd.env("MIRI_MAGIC_DIR", root_dir.clone());

let path = std::env::current_exe().expect("current executable path invalid");
cmd.env("RUSTC_WRAPPER", path);
if verbose {
Expand All @@ -470,27 +511,41 @@ fn in_cargo_miri() {
fn inside_cargo_rustc() {
let sysroot = std::env::var("MIRI_SYSROOT").expect("The wrapper should have set MIRI_SYSROOT");


let rustc_args = std::env::args().skip(2); // skip `cargo rustc`
let mut args: Vec<String> = rustc_args
.chain(Some("--sysroot".to_owned()))
.chain(Some(sysroot))
.collect();
args.splice(0..0, miri::miri_default_args().iter().map(ToString::to_string));

let in_build_script = is_build_dep(std::env::args().skip(2));

let mut args = if in_build_script {
rustc_args.collect()
} else {
let mut args: Vec<String> = rustc_args
.chain(Some("--sysroot".to_owned()))
.chain(Some(sysroot))
.collect();
args.splice(0..0, miri::miri_default_args().iter().map(ToString::to_string));
args
};

// See if we can find the `cargo-miri` markers. Those only get added to the binary we want to
// run. They also serve to mark the user-defined arguments, which we have to move all the way
// to the end (they get added somewhere in the middle).
let needs_miri = if let Some(begin) = args.iter().position(|arg| arg == "cargo-miri-marker-begin") {
let end = args
.iter()
.position(|arg| arg == "cargo-miri-marker-end")
.expect("cannot find end marker");
// These mark the user arguments. We remove the first and last as they are the markers.
let mut user_args = args.drain(begin..=end);
assert_eq!(user_args.next().unwrap(), "cargo-miri-marker-begin");
assert_eq!(user_args.next_back().unwrap(), "cargo-miri-marker-end");
// Collect the rest and add it back at the end.
let mut user_args = user_args.collect::<Vec<String>>();
let needs_miri = if is_target_crate(in_build_script) {
let raw_args = std::env::var("MIRI_MAGIC_ARGS").expect("Missing magic!");
let mut user_args = vec![];
let mut slice = raw_args.as_str();
loop {
match slice.find(';') {
Some(pos) => {
let len: usize = slice[..(pos)].parse().unwrap();
let arg = slice[(pos+1)..=(pos+len)].to_string();
user_args.push(arg);
slice = &slice[(pos+len+1)..];
},
None => break
}
}

args.append(&mut user_args);
// Run this in Miri.
true
Expand Down
2 changes: 1 addition & 1 deletion src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) {
};
e.print_backtrace();
if let Some(frame) = ecx.stack().last() {
let block = &frame.body.basic_blocks()[frame.block];
let block = &frame.body.basic_blocks()[frame.block.unwrap()];
let span = if frame.stmt < block.statements.len() {
block.statements[frame.stmt].source_info.span
} else {
Expand Down
17 changes: 13 additions & 4 deletions src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
args: &[OpTy<'tcx, Tag>],
dest: Option<PlaceTy<'tcx, Tag>>,
ret: Option<mir::BasicBlock>,
_unwind: Option<mir::BasicBlock>,
) -> InterpResult<'tcx, Option<&'mir mir::Body<'tcx>>> {
ecx.find_fn(instance, args, dest, ret)
}
Expand All @@ -194,8 +195,14 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
span: Span,
instance: ty::Instance<'tcx>,
args: &[OpTy<'tcx, Tag>],
dest: PlaceTy<'tcx, Tag>,
dest: Option<PlaceTy<'tcx, Tag>>,
_ret: Option<mir::BasicBlock>,
_unwind: Option<mir::BasicBlock>
) -> InterpResult<'tcx> {
let dest = match dest {
Some(dest) => dest,
None => throw_ub!(Unreachable)
};
ecx.call_intrinsic(span, instance, args, dest)
}

Expand Down Expand Up @@ -353,13 +360,15 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
fn stack_pop(
ecx: &mut InterpCx<'mir, 'tcx, Self>,
extra: stacked_borrows::CallId,
) -> InterpResult<'tcx> {
Ok(ecx
_unwinding: bool
) -> InterpResult<'tcx, StackPopInfo> {
ecx
.memory
.extra
.stacked_borrows
.borrow_mut()
.end_call(extra))
.end_call(extra);
Ok(StackPopInfo::Normal)
}

#[inline(always)]
Expand Down
2 changes: 1 addition & 1 deletion src/shims/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
mir,
Some(ret_place),
// Directly return to caller.
StackPopCleanup::Goto(Some(ret)),
StackPopCleanup::Goto { ret: Some(ret), unwind: None },
)?;
let mut args = this.frame().body.args_iter();

Expand Down
2 changes: 1 addition & 1 deletion src/shims/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
dest: PlaceTy<'tcx, Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
if this.emulate_intrinsic(span, instance, args, dest)? {
if this.emulate_intrinsic(span, instance, args, Some(dest))? {
return Ok(());
}
let tcx = &{this.tcx.tcx};
Expand Down
2 changes: 1 addition & 1 deletion src/shims/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
);

// First, run the common hooks also supported by CTFE.
if this.hook_fn(instance, args, dest)? {
if this.hook_panic_fn(instance, args, dest)? {
this.goto_block(ret)?;
return Ok(None);
}
Expand Down