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.

Sign up for GitHub

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

Add a magic comment syntax to set MIRIFLAGS #781

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add a magic comment syntax to set MIRIFLAGS #781

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
73f7c7c
Add a magic comment syntax to set MIRIFLAGS
saethlin Feb 21, 2022
e1c07ef
Don't default to diabling isolation, but demo it in docs
saethlin Feb 23, 2022
605f1fe
Parse magic comment on lines after the first if they are not indented
saethlin Feb 28, 2022
0e47158
Accept the comment anywhere
saethlin Feb 28, 2022
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: 0 additions & 1 deletion compiler/miri/cargo-miri-playground
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@
set -eu

export MIRI_SYSROOT=~/.cache/miri/HOST
export MIRIFLAGS="-Zmiri-disable-isolation"
exec cargo miri run
152 changes: 147 additions & 5 deletions ui/src/sandbox.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use regex::Regex;
use serde_derive::Deserialize;
use snafu::{ResultExt, Snafu};
use std::{ffi::OsStr, fmt, io, os::unix::fs::PermissionsExt, string, time::Duration};
Expand Down Expand Up @@ -280,6 +281,27 @@ fn set_execution_environment(
cmd.apply_backtrace(&req);
}

fn parse_magic_comments(code: &str) -> Vec<(&'static str, String)> {
lazy_static::lazy_static! {
pub static ref MIRIFLAGS: Regex = Regex::new(r#"(///|//!|//)\s*MIRIFLAGS\s*=\s*(.*)"#).unwrap();
Copy link

Choose a reason for hiding this comment

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

An outer attribute seems weird, here:

Suggested change
pub static ref MIRIFLAGS: Regex = Regex::new(r#"(///|//!|//)\s*MIRIFLAGS\s*=\s*(.*)"#).unwrap();
pub static ref MIRIFLAGS: Regex = Regex::new(r#"(//!|//)\s*MIRIFLAGS\s*=\s*(.*)"#).unwrap();

Copy link
Member Author

Choose a reason for hiding this comment

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

/// MIRIFLAGS=-Zmiri-tag-raw-pointers
fn main() {
}

seems plausible to me. My instinct here is to err on the side of accepting as much as possible, because I think the only alternative we have is silently ignoring things.

Copy link
Member

Choose a reason for hiding this comment

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

I agree... whenever I use magic comments on ui tests I am nervous that they might be silently ignored.

Copy link

Choose a reason for hiding this comment

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

So, the thing is that:

#![feature(…)]

/// MIRIFLAGS=-Zmiri-tag-raw-pointers
fn main() {

would fail, IIUC, since it wouldn't be the first-line of a file. And I can imagine the transition from your snippet to mine occurring quite easily, especially given that

/// MIRIFLAGS=-Zmiri-tag-raw-pointers
#![feature(…)]

fn main() {

would not be valid Rust.

I'd rather have Miri start saying with which MIRIFLAGS it is being run (which seems like a nice addition overall), rather than have an outer attribute be accepter where an inner one should be used: it is all too reminiscent of:

//! src/lib.rs
#[forbid(unsafe_code)]

use …;

fn foo… /* actually allowed to use `unsafe`! */

Copy link
Member

Choose a reason for hiding this comment

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

would fail, IIUC, since it wouldn't be the first-line of a file.

Hm... good point. Playground will also add feature lines at the very top of the snippet when you click the error message that complains about a missing feature gate.

Copy link
Member Author

@saethlin saethlin Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think #![feature(...)] is pretty problematic here. I was afraid magic comments would run into this kind of complexity.

Parsing the magic comment on lines other than the first makes some sense in response to this, but then we run into this:

// This example works totally fine by default. But it breaks if we set
// MIRIFLAGS=-Zmiri-tag-raw-pointers
fn main() {
    🤦 
}

I agree that Miri should report its configuration somewhere.

Other things we could do...

  1. Add a new UI element. I don't really want to do this because it's a lot of work, and also MIRIFLAGS is quite unique in that it needs to be provided as an environment variable when cargo-miri is run. All other build options we would want as arguments to cargo rustc.
  2. Try to parse the first comment line without leading whitespace. Is it sufficiently unlikely to accidentally set MIRIFLAGS this way?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is unreasonable. It's not like people are shipping code using the playground, and this seems to be quite an edge case (https://grep.app/search?q=//%20MIRIFLAGS and https://cs.github.com/?scopeName=All+repos&scope=&q=%22%2F%2F+MIRIFLAGS%22 say enough of one that it doesn't exist in open source rust code).

Copy link
Member Author

Choose a reason for hiding this comment

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

@thomcc Can you clarify what exactly "it" is here?

I don't think this is unreasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Parsing the magic comment on lines other than the first makes some sense in response to this, but then we run into this...

The issue you are describing here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine if that turns on that MIRIFLAG

}

fn match_of(line: &str, pat: &Regex) -> Option<String> {
pat.captures(line.trim())
.and_then(|caps| caps.get(2))
.map(|mat| mat.as_str().trim_matches(&['\'', '"'][..]).to_string())
saethlin marked this conversation as resolved.
Show resolved Hide resolved
}

let mut settings = Vec::new();
if let Some(first_line) = code.trim().lines().next() {
if let Some(miri_flags) = match_of(first_line, &*MIRIFLAGS) {
settings.push(("MIRIFLAGS", miri_flags));
}
}

settings
}

pub mod fut {
use snafu::prelude::*;
use std::{
Expand All @@ -292,9 +314,9 @@ pub mod fut {
use tokio::{fs, process::Command, time};

use super::{
basic_secure_docker_command, build_execution_command, set_execution_environment,
vec_to_str, wide_open_permissions, BacktraceRequest, Channel, ClippyRequest,
ClippyResponse, CompileRequest, CompileResponse, CompileTarget,
basic_secure_docker_command, build_execution_command, parse_magic_comments,
set_execution_environment, vec_to_str, wide_open_permissions, BacktraceRequest, Channel,
ClippyRequest, ClippyResponse, CompileRequest, CompileResponse, CompileTarget,
CompilerExecutionTimedOutSnafu, CrateInformation, CrateInformationInner, CrateType,
CrateTypeRequest, DemangleAssembly, DockerCommandExt, EditionRequest, ExecuteRequest,
ExecuteResponse, FormatRequest, FormatResponse, MacroExpansionRequest,
Expand Down Expand Up @@ -454,7 +476,9 @@ pub mod fut {

pub async fn miri(&self, req: &MiriRequest) -> Result<MiriResponse> {
self.write_source_code(&req.code).await?;
let command = self.miri_command(req);

let settings = parse_magic_comments(&req.code);
let command = self.miri_command(settings, req);

let output = run_command_with_timeout(command).await?;

Expand Down Expand Up @@ -651,10 +675,23 @@ pub mod fut {
cmd
}

fn miri_command(&self, req: impl EditionRequest) -> Command {
fn miri_command(
&self,
env_settings: Vec<(&'static str, String)>,
req: impl EditionRequest,
) -> Command {
let mut cmd = self.docker_command(None);
cmd.apply_edition(req);

let miri_env =
if let Some((_, flags)) = env_settings.iter().find(|(k, _)| k == &"MIRIFLAGS") {
format!("MIRIFLAGS={} -Zmiri-disable-isolation", flags)
Copy link
Member

@RalfJung RalfJung Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Changing the flags like this seems potentially confusing, in particular a flag like this that can heavily affect debugging (since it introduces non-determinism).

If people can set their own flag I am not sure if we even still need the default flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point.

I'm concerned that if this flag is removed by default, that people will find programs that used to work and don't anymore. How would we direct them to add the magic comment? I can definitely add something to the help page, but is that enough?

Copy link
Member

@thomcc thomcc Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Stuff already breaks on the playground over time, as crate versions change. I especially hope there's nobody relying on miri-in-playground not changing in any real way...

Edit: That said, I do kind of think -Zmiri-disable-isolation makes sense to be on by default in the playground, since it's already sandboxed.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to be really fancy, when Miri shows the error saying "pass -Zmiri-disable-isolation to run this operation", the playground could turn that into a link that adds the relevant magic command. ;) The playground has such links for other situations. Those are easier though since here, if there already is a MIRIFLAGS comment, one has to now change the existing comment...

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 could probably handle most cases. Would it be better to do this edit which is almost always correct, or append something to the output that points to documentation? Both options seem sort of bad in slight ways. I like that currently the playground just echoes cargo output.

} else {
"MIRIFLAGS=-Zmiri-disable-isolation".to_string()
};

cmd.args(&["--env", &miri_env]);

cmd.arg("miri").args(&["cargo", "miri-playground"]);

log::debug!("Miri command is {:?}", cmd);
Expand Down Expand Up @@ -1653,6 +1690,111 @@ mod test {
Ok(())
}

#[test]
fn magic_comments() {
let _singleton = one_test_at_a_time();

let expected = vec![("MIRIFLAGS", "-Zmiri-tag-raw-pointers".to_string())];

// Every comment syntax
let code = r#"//MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_magic_comments(code), expected);

let code = r#"///MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_magic_comments(code), expected);

let code = r#"//!MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_magic_comments(code), expected);

// Whitespace is ignored
let code = r#"// MIRIFLAGS = -Zmiri-tag-raw-pointers"#;
assert_eq!(parse_magic_comments(code), expected);

// Both quotes are stripped
let code = r#"//MIRIFLAGS='-Zmiri-tag-raw-pointers'"#;
assert_eq!(parse_magic_comments(code), expected);

let code = r#"//MIRIFLAGS="-Zmiri-tag-raw-pointers""#;
assert_eq!(parse_magic_comments(code), expected);

// Leading spaces are ignored
let code = r#"
//MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_magic_comments(code), expected);

let expected = vec![(
"MIRIFLAGS",
"-Zmiri-tag-raw-pointers -Zmiri-symbolic-alignment-check".to_string(),
)];

// Doesn't break up flags even if they contain whitespace
let code = r#"//MIRIFLAGS="-Zmiri-tag-raw-pointers -Zmiri-symbolic-alignment-check""#;
assert_eq!(parse_magic_comments(code), expected);

// Even if they aren't wrapped in quoted (possibly dubious)
let code = r#"//MIRIFLAGS=-Zmiri-tag-raw-pointers -Zmiri-symbolic-alignment-check"#;
assert_eq!(parse_magic_comments(code), expected);
}

#[test]
fn miriflags_work() -> Result<()> {
let _singleton = one_test_at_a_time();

let code = r#"
fn main() {
let a = 0u8;
let ptr = &a as *const u8 as usize as *const u8;
unsafe {
println!("{}", *ptr);
}
}
"#;

let req = MiriRequest {
code: code.to_string(),
edition: None,
};

let sb = Sandbox::new()?;
let resp = sb.miri(&req)?;

assert!(resp.success);

let code = r#"
// MIRIFLAGS=-Zmiri-tag-raw-pointers
fn main() {
let a = 0u8;
let ptr = &a as *const u8 as usize as *const u8;
unsafe {
println!("{}", *ptr);
}
}
"#;

let req = MiriRequest {
code: code.to_string(),
edition: None,
};

let sb = Sandbox::new()?;
let resp = sb.miri(&req)?;

assert!(!resp.success);

assert!(
resp.stderr
.contains("trying to reborrow for SharedReadOnly"),
"was: {}",
resp.stderr
);
assert!(
resp.stderr.contains("but parent tag"),
"was: {}",
resp.stderr
);
Ok(())
}

#[test]
fn network_connections_are_disabled() {
let _singleton = one_test_at_a_time();
Expand Down