From 9cba160d524698f6f9292589e45909b2450ff60b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 19 May 2024 18:03:13 +0200 Subject: [PATCH] use a little arg-parsing helper for miri-script --- src/tools/miri/miri-script/src/args.rs | 136 ++++++++++++++++++ src/tools/miri/miri-script/src/commands.rs | 74 ++++++---- src/tools/miri/miri-script/src/main.rs | 158 +++++++++------------ src/tools/miri/miri-script/src/util.rs | 34 +---- src/tools/miri/src/bin/miri.rs | 5 +- 5 files changed, 261 insertions(+), 146 deletions(-) create mode 100644 src/tools/miri/miri-script/src/args.rs diff --git a/src/tools/miri/miri-script/src/args.rs b/src/tools/miri/miri-script/src/args.rs new file mode 100644 index 00000000000..16a21757b35 --- /dev/null +++ b/src/tools/miri/miri-script/src/args.rs @@ -0,0 +1,136 @@ +use std::env; +use std::iter; + +use anyhow::{bail, Result}; + +pub struct Args { + args: iter::Peekable, + /// Set to `true` once we saw a `--`. + terminated: bool, +} + +impl Args { + pub fn new() -> Self { + let mut args = Args { args: env::args().peekable(), terminated: false }; + args.args.next().unwrap(); // skip program name + args + } + + /// Get the next argument without any interpretation. + pub fn next_raw(&mut self) -> Option { + self.args.next() + } + + /// Consume a `-$f` flag if present. + pub fn get_short_flag(&mut self, flag: char) -> Result { + if self.terminated { + return Ok(false); + } + if let Some(next) = self.args.peek() { + if let Some(next) = next.strip_prefix("-") { + if let Some(next) = next.strip_prefix(flag) { + if next.is_empty() { + self.args.next().unwrap(); // consume this argument + return Ok(true); + } else { + bail!("`-{flag}` followed by value"); + } + } + } + } + Ok(false) + } + + /// Consume a `--$name` flag if present. + pub fn get_long_flag(&mut self, name: &str) -> Result { + if self.terminated { + return Ok(false); + } + if let Some(next) = self.args.peek() { + if let Some(next) = next.strip_prefix("--") { + if next == name { + self.args.next().unwrap(); // consume this argument + return Ok(true); + } + } + } + Ok(false) + } + + /// Consume a `--$name val` or `--$name=val` option if present. + pub fn get_long_opt(&mut self, name: &str) -> Result> { + assert!(!name.is_empty()); + if self.terminated { + return Ok(None); + } + let Some(next) = self.args.peek() else { return Ok(None) }; + let Some(next) = next.strip_prefix("--") else { return Ok(None) }; + let Some(next) = next.strip_prefix(name) else { return Ok(None) }; + // Starts with `--flag`. + Ok(if let Some(val) = next.strip_prefix("=") { + // `--flag=val` form + let val = val.into(); + self.args.next().unwrap(); // consume this argument + Some(val) + } else if next.is_empty() { + // `--flag val` form + self.args.next().unwrap(); // consume this argument + let Some(val) = self.args.next() else { bail!("`--{name}` not followed by value") }; + Some(val) + } else { + // Some unrelated flag, like `--flag-more` or so. + None + }) + } + + /// Consume a `--$name=val` or `--$name` option if present; the latter + /// produces a default value. (`--$name val` is *not* accepted for this form + /// of argument, it understands `val` already as the next argument!) + pub fn get_long_opt_with_default( + &mut self, + name: &str, + default: &str, + ) -> Result> { + assert!(!name.is_empty()); + if self.terminated { + return Ok(None); + } + let Some(next) = self.args.peek() else { return Ok(None) }; + let Some(next) = next.strip_prefix("--") else { return Ok(None) }; + let Some(next) = next.strip_prefix(name) else { return Ok(None) }; + // Starts with `--flag`. + Ok(if let Some(val) = next.strip_prefix("=") { + // `--flag=val` form + let val = val.into(); + self.args.next().unwrap(); // consume this argument + Some(val) + } else if next.is_empty() { + // `--flag` form + self.args.next().unwrap(); // consume this argument + Some(default.into()) + } else { + // Some unrelated flag, like `--flag-more` or so. + None + }) + } + + /// Returns the next free argument or uninterpreted flag, or `None` if there are no more + /// arguments left. `--` is returned as well, but it is interpreted in the sense that no more + /// flags will be parsed after this. + pub fn get_other(&mut self) -> Option { + if self.terminated { + return self.args.next(); + } + let next = self.args.next()?; + if next == "--" { + self.terminated = true; // don't parse any more flags + // This is where our parser is special, we do yield the `--`. + } + Some(next) + } + + /// Return the rest of the aguments entirely unparsed. + pub fn remainder(self) -> Vec { + self.args.collect() + } +} diff --git a/src/tools/miri/miri-script/src/commands.rs b/src/tools/miri/miri-script/src/commands.rs index 7b5a047bf75..8061f99b5d6 100644 --- a/src/tools/miri/miri-script/src/commands.rs +++ b/src/tools/miri/miri-script/src/commands.rs @@ -25,7 +25,11 @@ impl MiriEnv { /// Returns the location of the sysroot. /// /// If the target is None the sysroot will be built for the host machine. - fn build_miri_sysroot(&mut self, quiet: bool, target: Option<&OsStr>) -> Result { + fn build_miri_sysroot( + &mut self, + quiet: bool, + target: Option>, + ) -> Result { if let Some(miri_sysroot) = self.sh.var_os("MIRI_SYSROOT") { // Sysroot already set, use that. return Ok(miri_sysroot.into()); @@ -37,14 +41,17 @@ impl MiriEnv { self.build(path!(self.miri_dir / "Cargo.toml"), &[], quiet)?; self.build(&manifest_path, &[], quiet)?; - let target_flag = - if let Some(target) = target { vec![OsStr::new("--target"), target] } else { vec![] }; + let target_flag = if let Some(target) = &target { + vec![OsStr::new("--target"), target.as_ref()] + } else { + vec![] + }; let target_flag = &target_flag; if !quiet { eprint!("$ cargo miri setup"); - if let Some(target) = target { - eprint!(" --target {target}", target = target.to_string_lossy()); + if let Some(target) = &target { + eprint!(" --target {target}", target = target.as_ref().to_string_lossy()); } eprintln!(); } @@ -157,8 +164,8 @@ impl Command { Command::Build { flags } => Self::build(flags), Command::Check { flags } => Self::check(flags), Command::Test { bless, flags, target } => Self::test(bless, flags, target), - Command::Run { dep, verbose, many_seeds, flags } => - Self::run(dep, verbose, many_seeds, flags), + Command::Run { dep, verbose, many_seeds, target, edition, flags } => + Self::run(dep, verbose, many_seeds, target, edition, flags), Command::Fmt { flags } => Self::fmt(flags), Command::Clippy { flags } => Self::clippy(flags), Command::Cargo { flags } => Self::cargo(flags), @@ -169,7 +176,7 @@ impl Command { } } - fn toolchain(flags: Vec) -> Result<()> { + fn toolchain(flags: Vec) -> Result<()> { // Make sure rustup-toolchain-install-master is installed. which::which("rustup-toolchain-install-master") .context("Please install rustup-toolchain-install-master by running 'cargo install rustup-toolchain-install-master'")?; @@ -364,7 +371,7 @@ impl Command { Ok(()) } - fn bench(target: Option, benches: Vec) -> Result<()> { + fn bench(target: Option, benches: Vec) -> Result<()> { // The hyperfine to use let hyperfine = env::var("HYPERFINE"); let hyperfine = hyperfine.as_deref().unwrap_or("hyperfine -w 1 -m 5 --shell=none"); @@ -378,14 +385,14 @@ impl Command { let sh = Shell::new()?; sh.change_dir(miri_dir()?); let benches_dir = "bench-cargo-miri"; - let benches = if benches.is_empty() { + let benches: Vec = if benches.is_empty() { sh.read_dir(benches_dir)? .into_iter() .filter(|path| path.is_dir()) .map(Into::into) .collect() } else { - benches.to_owned() + benches.into_iter().map(Into::into).collect() }; let target_flag = if let Some(target) = target { let mut flag = OsString::from("--target="); @@ -409,28 +416,28 @@ impl Command { Ok(()) } - fn install(flags: Vec) -> Result<()> { + fn install(flags: Vec) -> Result<()> { let e = MiriEnv::new()?; e.install_to_sysroot(e.miri_dir.clone(), &flags)?; e.install_to_sysroot(path!(e.miri_dir / "cargo-miri"), &flags)?; Ok(()) } - fn build(flags: Vec) -> Result<()> { + fn build(flags: Vec) -> Result<()> { let e = MiriEnv::new()?; e.build(path!(e.miri_dir / "Cargo.toml"), &flags, /* quiet */ false)?; e.build(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags, /* quiet */ false)?; Ok(()) } - fn check(flags: Vec) -> Result<()> { + fn check(flags: Vec) -> Result<()> { let e = MiriEnv::new()?; e.check(path!(e.miri_dir / "Cargo.toml"), &flags)?; e.check(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags)?; Ok(()) } - fn clippy(flags: Vec) -> Result<()> { + fn clippy(flags: Vec) -> Result<()> { let e = MiriEnv::new()?; e.clippy(path!(e.miri_dir / "Cargo.toml"), &flags)?; e.clippy(path!(e.miri_dir / "cargo-miri" / "Cargo.toml"), &flags)?; @@ -438,7 +445,7 @@ impl Command { Ok(()) } - fn cargo(flags: Vec) -> Result<()> { + fn cargo(flags: Vec) -> Result<()> { let e = MiriEnv::new()?; let toolchain = &e.toolchain; // We carefully kept the working dir intact, so this will run cargo *on the workspace in the @@ -447,7 +454,7 @@ impl Command { Ok(()) } - fn test(bless: bool, mut flags: Vec, target: Option) -> Result<()> { + fn test(bless: bool, mut flags: Vec, target: Option) -> Result<()> { let mut e = MiriEnv::new()?; // Prepare a sysroot. @@ -475,21 +482,30 @@ impl Command { dep: bool, verbose: bool, many_seeds: Option>, - mut flags: Vec, + target: Option, + edition: Option, + flags: Vec, ) -> Result<()> { let mut e = MiriEnv::new()?; - let target = arg_flag_value(&flags, "--target"); + // More flags that we will pass before `flags` + // (because `flags` may contain `--`). + let mut early_flags = Vec::::new(); - // Scan for "--edition", set one ourselves if that flag is not present. - let have_edition = arg_flag_value(&flags, "--edition").is_some(); - if !have_edition { - flags.push("--edition=2021".into()); // keep in sync with `tests/ui.rs`.` + // Add target, edition to flags. + if let Some(target) = &target { + early_flags.push("--target".into()); + early_flags.push(target.into()); } + if verbose { + early_flags.push("--verbose".into()); + } + early_flags.push("--edition".into()); + early_flags.push(edition.as_deref().unwrap_or("2021").into()); - // Prepare a sysroot, and add it to the flags. + // Prepare a sysroot, add it to the flags. let miri_sysroot = e.build_miri_sysroot(/* quiet */ !verbose, target.as_deref())?; - flags.push("--sysroot".into()); - flags.push(miri_sysroot.into()); + early_flags.push("--sysroot".into()); + early_flags.push(miri_sysroot.into()); // Compute everything needed to run the actual command. Also add MIRIFLAGS. let miri_manifest = path!(e.miri_dir / "Cargo.toml"); @@ -515,7 +531,7 @@ impl Command { }; cmd.set_quiet(!verbose); // Add Miri flags - let cmd = cmd.args(&miri_flags).args(seed_flag).args(&flags); + let cmd = cmd.args(&miri_flags).args(&seed_flag).args(&early_flags).args(&flags); // And run the thing. Ok(cmd.run()?) }; @@ -534,7 +550,7 @@ impl Command { Ok(()) } - fn fmt(flags: Vec) -> Result<()> { + fn fmt(flags: Vec) -> Result<()> { use itertools::Itertools; let e = MiriEnv::new()?; @@ -556,6 +572,6 @@ impl Command { .filter_ok(|item| item.file_type().is_file()) .map_ok(|item| item.into_path()); - e.format_files(files, &e.toolchain[..], &config_path, &flags[..]) + e.format_files(files, &e.toolchain[..], &config_path, &flags) } } diff --git a/src/tools/miri/miri-script/src/main.rs b/src/tools/miri/miri-script/src/main.rs index a8626ceb45d..d436ef0c5aa 100644 --- a/src/tools/miri/miri-script/src/main.rs +++ b/src/tools/miri/miri-script/src/main.rs @@ -1,10 +1,10 @@ #![allow(clippy::needless_question_mark)] +mod args; mod commands; mod util; -use std::ffi::OsString; -use std::{env, ops::Range}; +use std::ops::Range; use anyhow::{anyhow, bail, Context, Result}; @@ -16,26 +16,26 @@ pub enum Command { /// sysroot, to prevent conflicts with other toolchains. Install { /// Flags that are passed through to `cargo install`. - flags: Vec, + flags: Vec, }, /// Just build miri. Build { /// Flags that are passed through to `cargo build`. - flags: Vec, + flags: Vec, }, /// Just check miri. Check { /// Flags that are passed through to `cargo check`. - flags: Vec, + flags: Vec, }, /// Build miri, set up a sysroot and then run the test suite. Test { bless: bool, /// The cross-interpretation target. /// If none then the host is the target. - target: Option, + target: Option, /// Flags that are passed through to the test harness. - flags: Vec, + flags: Vec, }, /// Build miri, set up a sysroot and then run the driver with the given . /// (Also respects MIRIFLAGS environment variable.) @@ -43,33 +43,35 @@ pub enum Command { dep: bool, verbose: bool, many_seeds: Option>, + target: Option, + edition: Option, /// Flags that are passed through to `miri`. - flags: Vec, + flags: Vec, }, /// Format all sources and tests. Fmt { /// Flags that are passed through to `rustfmt`. - flags: Vec, + flags: Vec, }, /// Runs clippy on all sources. Clippy { /// Flags that are passed through to `cargo clippy`. - flags: Vec, + flags: Vec, }, /// Runs just `cargo ` with the Miri-specific environment variables. /// Mainly meant to be invoked by rust-analyzer. - Cargo { flags: Vec }, + Cargo { flags: Vec }, /// Runs the benchmarks from bench-cargo-miri in hyperfine. hyperfine needs to be installed. Bench { - target: Option, + target: Option, /// List of benchmarks to run. By default all benchmarks are run. - benches: Vec, + benches: Vec, }, /// Update and activate the rustup toolchain 'miri' to the commit given in the /// `rust-version` file. /// `rustup-toolchain-install-master` must be installed for this to work. Any extra /// flags are passed to `rustup-toolchain-install-master`. - Toolchain { flags: Vec }, + Toolchain { flags: Vec }, /// Pull and merge Miri changes from the rustc repo. Defaults to fetching the latest /// rustc commit. The fetched commit is stored in the `rust-version` file, so the /// next `./miri toolchain` will install the rustc that just got pulled. @@ -145,113 +147,95 @@ Pass extra flags to all cargo invocations. (Ignored by `./miri cargo`.)"#; fn main() -> Result<()> { // We are hand-rolling our own argument parser, since `clap` can't express what we need // (https://github.com/clap-rs/clap/issues/5055). - let mut args = env::args_os().peekable(); - args.next().unwrap(); // skip program name - let command = match args.next().and_then(|s| s.into_string().ok()).as_deref() { - Some("build") => Command::Build { flags: args.collect() }, - Some("check") => Command::Check { flags: args.collect() }, + let mut args = args::Args::new(); + let command = match args.next_raw().as_deref() { + Some("build") => Command::Build { flags: args.remainder() }, + Some("check") => Command::Check { flags: args.remainder() }, Some("test") => { let mut target = None; let mut bless = false; - - while let Some(arg) = args.peek().and_then(|s| s.to_str()) { - match arg { - "--bless" => bless = true, - "--target" => { - // Skip "--target" - args.next().unwrap(); - // Next argument is the target triple. - let val = args.peek().ok_or_else(|| { - anyhow!("`--target` must be followed by target triple") - })?; - target = Some(val.to_owned()); - } - // Only parse the leading flags. - _ => break, + let mut flags = Vec::new(); + loop { + if args.get_long_flag("bless")? { + bless = true; + } else if let Some(val) = args.get_long_opt("target")? { + target = Some(val); + } else if let Some(flag) = args.get_other() { + flags.push(flag); + } else { + break; } - - // Consume the flag, look at the next one. - args.next().unwrap(); } - - Command::Test { bless, flags: args.collect(), target } + Command::Test { bless, flags, target } } Some("run") => { let mut dep = false; let mut verbose = false; let mut many_seeds = None; - while let Some(arg) = args.peek().and_then(|s| s.to_str()) { - if arg == "--dep" { + let mut target = None; + let mut edition = None; + let mut flags = Vec::new(); + loop { + if args.get_long_flag("dep")? { dep = true; - } else if arg == "-v" || arg == "--verbose" { + } else if args.get_long_flag("verbose")? || args.get_short_flag('v')? { verbose = true; - } else if arg == "--many-seeds" { - many_seeds = Some(0..256); - } else if let Some(val) = arg.strip_prefix("--many-seeds=") { + } else if let Some(val) = args.get_long_opt_with_default("many-seeds", "0..256")? { let (from, to) = val.split_once("..").ok_or_else(|| { - anyhow!("invalid format for `--many-seeds`: expected `from..to`") + anyhow!("invalid format for `--many-seeds-range`: expected `from..to`") })?; let from: u32 = if from.is_empty() { 0 } else { - from.parse().context("invalid `from` in `--many-seeds=from..to")? + from.parse().context("invalid `from` in `--many-seeds-range=from..to")? }; - let to: u32 = to.parse().context("invalid `to` in `--many-seeds=from..to")?; + let to: u32 = + to.parse().context("invalid `to` in `--many-seeds-range=from..to")?; many_seeds = Some(from..to); + } else if let Some(val) = args.get_long_opt("target")? { + target = Some(val); + } else if let Some(val) = args.get_long_opt("edition")? { + edition = Some(val); + } else if let Some(flag) = args.get_other() { + flags.push(flag); } else { - break; // not for us + break; } - // Consume the flag, look at the next one. - args.next().unwrap(); } - Command::Run { dep, verbose, many_seeds, flags: args.collect() } + Command::Run { dep, verbose, many_seeds, target, edition, flags } } - Some("fmt") => Command::Fmt { flags: args.collect() }, - Some("clippy") => Command::Clippy { flags: args.collect() }, - Some("cargo") => Command::Cargo { flags: args.collect() }, - Some("install") => Command::Install { flags: args.collect() }, + Some("fmt") => Command::Fmt { flags: args.remainder() }, + Some("clippy") => Command::Clippy { flags: args.remainder() }, + Some("cargo") => Command::Cargo { flags: args.remainder() }, + Some("install") => Command::Install { flags: args.remainder() }, Some("bench") => { let mut target = None; - while let Some(arg) = args.peek().and_then(|s| s.to_str()) { - match arg { - "--target" => { - // Skip "--target" - args.next().unwrap(); - // Next argument is the target triple. - let val = args.peek().ok_or_else(|| { - anyhow!("`--target` must be followed by target triple") - })?; - target = Some(val.to_owned()); - } - // Only parse the leading flags. - _ => break, + let mut benches = Vec::new(); + loop { + if let Some(val) = args.get_long_opt("target")? { + target = Some(val); + } else if let Some(flag) = args.get_other() { + benches.push(flag); + } else { + break; } - - // Consume the flag, look at the next one. - args.next().unwrap(); } - - Command::Bench { target, benches: args.collect() } + Command::Bench { target, benches } } - Some("toolchain") => Command::Toolchain { flags: args.collect() }, + Some("toolchain") => Command::Toolchain { flags: args.remainder() }, Some("rustc-pull") => { - let commit = args.next().map(|a| a.to_string_lossy().into_owned()); - if args.next().is_some() { + let commit = args.next_raw(); + if args.next_raw().is_some() { bail!("Too many arguments for `./miri rustc-pull`"); } Command::RustcPull { commit } } Some("rustc-push") => { - let github_user = args - .next() - .ok_or_else(|| { - anyhow!("Missing first argument for `./miri rustc-push GITHUB_USER [BRANCH]`") - })? - .to_string_lossy() - .into_owned(); - let branch = - args.next().unwrap_or_else(|| "miri-sync".into()).to_string_lossy().into_owned(); - if args.next().is_some() { + let github_user = args.next_raw().ok_or_else(|| { + anyhow!("Missing first argument for `./miri rustc-push GITHUB_USER [BRANCH]`") + })?; + let branch = args.next_raw().unwrap_or_else(|| "miri-sync".into()); + if args.next_raw().is_some() { bail!("Too many arguments for `./miri rustc-push GITHUB_USER BRANCH`"); } Command::RustcPush { github_user, branch } diff --git a/src/tools/miri/miri-script/src/util.rs b/src/tools/miri/miri-script/src/util.rs index 2b5791f3ea5..e9095a45fce 100644 --- a/src/tools/miri/miri-script/src/util.rs +++ b/src/tools/miri/miri-script/src/util.rs @@ -27,30 +27,6 @@ pub fn flagsplit(flags: &str) -> Vec { flags.split(' ').map(str::trim).filter(|s| !s.is_empty()).map(str::to_string).collect() } -pub fn arg_flag_value( - args: impl IntoIterator>, - flag: &str, -) -> Option { - let mut args = args.into_iter(); - while let Some(arg) = args.next() { - let arg = arg.as_ref(); - if arg == "--" { - return None; - } - let Some(arg) = arg.to_str() else { - // Skip non-UTF-8 arguments. - continue; - }; - if arg == flag { - // Next one is the value. - return Some(args.next()?.as_ref().to_owned()); - } else if let Some(val) = arg.strip_prefix(flag).and_then(|s| s.strip_prefix("=")) { - return Some(val.to_owned().into()); - } - } - None -} - /// Some extra state we track for building Miri, such as the right RUSTFLAGS. pub struct MiriEnv { /// miri_dir is the root of the miri repository checkout we are working in. @@ -133,7 +109,7 @@ impl MiriEnv { pub fn build( &self, manifest_path: impl AsRef, - args: &[OsString], + args: &[String], quiet: bool, ) -> Result<()> { let MiriEnv { toolchain, cargo_extra_flags, .. } = self; @@ -149,21 +125,21 @@ impl MiriEnv { Ok(()) } - pub fn check(&self, manifest_path: impl AsRef, args: &[OsString]) -> Result<()> { + pub fn check(&self, manifest_path: impl AsRef, args: &[String]) -> Result<()> { let MiriEnv { toolchain, cargo_extra_flags, .. } = self; cmd!(self.sh, "cargo +{toolchain} check {cargo_extra_flags...} --manifest-path {manifest_path} --all-targets {args...}") .run()?; Ok(()) } - pub fn clippy(&self, manifest_path: impl AsRef, args: &[OsString]) -> Result<()> { + pub fn clippy(&self, manifest_path: impl AsRef, args: &[String]) -> Result<()> { let MiriEnv { toolchain, cargo_extra_flags, .. } = self; cmd!(self.sh, "cargo +{toolchain} clippy {cargo_extra_flags...} --manifest-path {manifest_path} --all-targets {args...}") .run()?; Ok(()) } - pub fn test(&self, manifest_path: impl AsRef, args: &[OsString]) -> Result<()> { + pub fn test(&self, manifest_path: impl AsRef, args: &[String]) -> Result<()> { let MiriEnv { toolchain, cargo_extra_flags, .. } = self; cmd!( self.sh, @@ -181,7 +157,7 @@ impl MiriEnv { files: impl Iterator>, toolchain: &str, config_path: &Path, - flags: &[OsString], + flags: &[String], ) -> anyhow::Result<()> { use itertools::Itertools; diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 305e9cd8d34..d748febeed4 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -405,9 +405,12 @@ fn main() { let mut rustc_args = vec![]; let mut after_dashdash = false; - // If user has explicitly enabled/disabled isolation let mut isolation_enabled: Option = None; + + // Note that we require values to be given with `=`, not with a space. + // This matches how rustc parses `-Z`. + // However, unlike rustc we do not accept a space after `-Z`. for arg in args { if rustc_args.is_empty() { // Very first arg: binary name.