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

Enable miri in CI and run miri tests in CI #698

Open
kkysen opened this issue Oct 10, 2022 · 3 comments
Open

Enable miri in CI and run miri tests in CI #698

kkysen opened this issue Oct 10, 2022 · 3 comments

Comments

@kkysen
Copy link
Contributor

kkysen commented Oct 10, 2022

In #686, I tried to add a miri test, which would be run in CI like the others, but I get a permission denied error from miri running in CI when or right after it installs xargo. Furthermore, it'd probably be a good idea to install xargo beforehand so it doesn't have to be re-installed every time. The miri test is #[ignore]d for now, but ideally, it'd be enabled so we don't accidentally re-introduce UB and flakiness into the analysis/test tests.

kkysen added a commit that referenced this issue Oct 10, 2022
Fixes #685.

This makes `analysis/test` `miri`-compatible when running with `--features miri`.  This is done by using a monomorphic `printf` shim, since `miri` can't handle variadic functions like `printf`.  Since all uses of `printf` in `analysis/test` are monomorphic (they all have the same format string), we can substitute a `fn printf` that is non-`extern "C"`, non-variadic (and thus `miri`-compatible), and that still has the same behavior for its call sites.

Then we add a test in `c2rust-pdg` that runs `miri` on `analysis/test` to ensure it stays UB-free.

However, we don't yet run this test by default (it's `#[ignore]`d for now) as there are issues with running `miri` in CI (it installs `xargo` every time and I'm getting a permission denied error (not sure from quite what exactly), and it'd be better to install `xargo` upfront, not on every run).  Thus, I'm `#[ignore]`ing it for now in 4152d34.  We can get it to run in CI correctly later in another PR, but I want to merge this now and avoid over-complicating it here.  The test can still be manually run with `cargo test -p c2rust-pdg -- --ignored analysis_test_miri`.  See #698 for the tracking issue to re-enable this test by default.

The new test passing is blocked on:
* #683 (f ixes #680)
* #684 (f ixes #681)

It would also be nice to f ix #682, but that's not completely necessary for this (though it would create a much less noisy output).
@bytewife
Copy link
Contributor

bytewife commented Jan 4, 2023

Hi! Where is the miri test located?

@kkysen
Copy link
Contributor Author

kkysen commented Jan 4, 2023

The miri test is here:

c2rust/pdg/src/main.rs

Lines 387 to 397 in 5ce9db6

#[test]
#[ignore]
fn analysis_tests_misc_miri() -> eyre::Result<()> {
init();
let mut cmd = Command::new("cargo");
cmd.current_dir(repo_dir()?.join("analysis/tests/misc"))
.args(&["miri", "run", "--features", "miri"])
.env("MIRIFLAGS", "");
let status = cmd.status()?;
ensure!(status.success(), eyre!("{cmd:?} failed: {status}"));
Ok(())

You can run it with cargo test -p c2rust-pdg -- --ignored tests::analysis_tests_misc_miri.

@kkysen
Copy link
Contributor Author

kkysen commented Jan 4, 2023

miri seems to longer use xargo as of rust-lang/miri#2564, so if we upgrade our nightly version past around nightly-2022-09-24, we should sidestep the xargo installation issue. We'll see if there's still an issue after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants