Skip to content

Add a magic comment syntax to set MIRIFLAGS #781

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

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 all commits
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
7 changes: 5 additions & 2 deletions ui/frontend/Help.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ const CLIPPY_EXAMPLE = `fn main() {
}
}`;

const MIRI_EXAMPLE = `fn main() {
const MIRI_EXAMPLE = `// MIRIFLAGS=-Zmiri-disable-isolation
fn main() {
let mut a: [u8; 0] = [];
unsafe {
*a.get_unchecked_mut(1) = 1;
Expand Down Expand Up @@ -154,7 +155,9 @@ const Help: React.SFC = () => {
<a href={MIRI_URL}>Miri</a> is an interpreter for Rust’s mid-level intermediate
representation (MIR) and can be used to detect certain kinds of undefined behavior
in your unsafe Rust code. Click on the <strong>Miri</strong> button in
the <strong>Tools</strong> menu to check.
the <strong>Tools</strong> menu to check. When running code with Miri, if the first
source line is a comment following a bash-like syntax for setting environment
variables, it will be used to set the <Code>MIRIFLAGS</Code> environment variable.
</p>

<Example code={MIRI_EXAMPLE} />
Expand Down
141 changes: 133 additions & 8 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_miri_flags(code: &str) -> Option<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)
.and_then(|caps| caps.get(2))
.map(|mat| mat.as_str().trim_matches(&['\'', '"'][..]).to_string())
}

for line in code.trim().lines() {
let line = line.trim();
if let Some(miri_flags) = match_of(line, &*MIRIFLAGS) {
return Some(miri_flags);
}
}

None
}

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_miri_flags,
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 miri_flags = parse_miri_flags(&req.code);
let command = self.miri_command(miri_flags, req);

let output = run_command_with_timeout(command).await?;

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

fn miri_command(&self, req: impl EditionRequest) -> Command {
fn miri_command(&self, miri_flags: Option<String>, req: impl EditionRequest) -> Command {
let mut cmd = self.docker_command(None);
cmd.apply_edition(req);

if let Some(flags) = miri_flags {
cmd.args(&["--env", &format!("MIRIFLAGS={}", flags)]);
}

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

log::debug!("Miri command is {:?}", cmd);
Expand Down Expand Up @@ -1636,17 +1664,114 @@ mod test {

assert!(
resp.stderr
.contains("pointer must be in-bounds at offset 1"),
.contains("has size 0, so pointer to 1 byte starting at offset 0 is out-of-bounds"),
"was: {}",
resp.stderr
);
Ok(())
}

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

let expected = Some("-Zmiri-tag-raw-pointers".to_string());

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

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

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

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

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

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

// Leading code is ignored
let code = r#"
fn main() {}
//MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_miri_flags(code), expected);

// Leading whitespace is ignored, flags still parse
let code = r#" //MIRIFLAGS=-Zmiri-tag-raw-pointers"#;
assert_eq!(parse_miri_flags(code), expected);

let expected = Some("-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_miri_flags(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_miri_flags(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("outside bounds of alloc"),
resp.stderr
.contains("trying to reborrow for SharedReadOnly"),
"was: {}",
resp.stderr
);
assert!(
resp.stderr.contains("which has size 0"),
resp.stderr.contains("but parent tag"),
"was: {}",
resp.stderr
);
Expand Down