Stop ignoring miri leaks

[jonhoo]

Now that rust-lang/miri#940 has been fixed,
and has landed in nightly
(rust-lang/rust#70897), we should be able to run
miri with leak check enabled again!

---

This change is 

[codecov]

Codecov Report

Merging #82 into master will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
src/map.rs	84.55% <ø> (+0.36%)	⬆️
src/node.rs	80.07% <0.00%> (+0.52%)	⬆️
src/raw/mod.rs	89.32% <0.00%> (+1.94%)	⬆️

[jonhoo]

@domenicquirl err, got a spurious SIGSEGV on the test suite here :/
https://dev.azure.com/jonhoo/jonhoo/_build/results?buildId=1068&view=logs&j=d84406a3-b0d8-5688-9074-4fc79d957ac9&t=d3f3159f-94fd-553d-ea32-0524cc49969a&l=132
Can you try running the tests in a loop with your latest tree stuff and see if it will occasionally crash?

[domenicquirl]

Oh noes! The location of the error in the Linux build you linked looks indeed like it could be a tree test. However, I see you have disabled the concurrent one if miri is active, so it would have to be a single-threaded one. The miri task fails in the disallow_evil test (which seems to have slipped into the treebin test mod, sorry 'bout that), though that error says it is "experimental", not sure what that's about.

Either way, I've just had cargo test run for about 15 minutes in a loop on master, but could not reproduce the error (Windows). Have you tried on a Linux machine, or maybe with nightly? Have you seen it occur on other platforms/toolchains while working on this?

[jonhoo]

@domenicquirl The error actually occurred in the normal test suite, not the miri one, so could still be the concurrent tree test. The failure was on nightly Linux, so I'll try running it in a loop on my computer too.

Looks like miri still thinks there is a leak. I think until crossbeam runs their tests through miri's leak check successfully, we shouldn't rely on it either.

[jonhoo]

@domenicquirl managed to reproduce the failed test here: #83

[domenicquirl]

Yeah, it looks like every defer inside crossbeam gets tagged as a leak. I guess at least miri did not detect any additional leaks (where we forget to defer_destroy something)? But we should probably just re-purpose this to exclude concurrent tests from miri (which is also kind of sad given that they are a huge part of flurry and our tests).

[jonhoo]

Hmm, it looks like currently we get stuck on tkaitchuck/aHash#32, so we need to wait for a resolution to that. I wonder how miri stopped working so completely without us noticing?

[domenicquirl]

It did run successfully in #79, which is not that old...

[jonhoo]

I think it may actually have failed to run, maybe due to some incorrect arguments? Because its log output is empty...

[jonhoo]

@RalfJung Something weird is going on here now. If I try to run the miri tests, it seems to get stuck somewhere when running the tests. Unfortunately, I cannot pass --nocapture to make it print the name of the next test it's going to run, since miri does not seem to recognize that argument. Using a mix of -v and the regular test order, I believe the issue is the hasher tests in tests/basic_ref.rs, but I cannot get miri to run just that test (or even just tests/basic_ref.rs). It seems to always insist to at least run the --lib tests?

$ cargo +nightly miri test --test basic_ref

runs the --lib and basic_ref tests seemingly concurrently, making it very hard to recognize where the hanging test is going. Or indeed to get any useful debug output. Trying to run just the hasher tests gives:

$ cargo +nightly miri test hasher
error: Found argument 'hasher' which wasn't expected, or isn't valid in this context

USAGE:
    cargo check --lib --profile <PROFILE-NAME>

For more information try --help

Is there a good other way for me to figure out which test is hanging?

[RalfJung]

Maybe the arguments get sent to the wrong program? Quoting from the docs:

You can pass arguments to Miri after the first --, and pass arguments to the interpreted program or test suite after the second --.

I never used --lib or so, but filtering works fine for me with cargo miri test -- -- filter.

[RalfJung]

Oh also when concurrency is involved and things get stuck, maybe you are running into rust-lang/miri#1388.

[RalfJung]

Unfortunately, I cannot pass --nocapture to make it print the name of the next test it's going to run, since miri does not seem to recognize that argument.

This is a test suite argument, right? Miri doesn't do anything there, so probably that is also just a case of not enough --.

One issue is that unfortunately cargo line-buffers Miri's output, so while a test runs we cannot yet see its name. I don't know how to make cargo not do that -- cargo thinks it is running rustc when it actually runs Miri, so line-buffering makes sense for it. I don't know any other way to hook into cargo than overwriting the RUSTC env var.

One thing I sometimes do is to run cargo miri test -v, which will print the invocation of the underlying Miri driver:

"/home/r/.cargo/bin/miri" "-Zalways-encode-mir" "-Zmir-emit-retag" "-Zmir-opt-level=0" "--cfg=miri" "-Cdebug-assertions=on" "--crate-name" "test" "--edition=2018" "tests/test.rs" "--error-format=json" "--json=diagnostic-rendered-ansi" "--emit=dep-info,metadata" "-C" "debuginfo=2" "--test" "-C" "metadata=63848be0a524aed1" "-C" "extra-filename=-63848be0a524aed1" "--out-dir" "/home/r/src/rust/miri/test-cargo-miri/target/debug/deps" "-C" "incremental=/home/r/src/rust/miri/test-cargo-miri/target/debug/incremental" "-L" "dependency=/home/r/src/rust/miri/test-cargo-miri/target/debug/deps" "--extern" "byteorder=/home/r/src/rust/miri/test-cargo-miri/target/debug/deps/libbyteorder-76227ec053bbc4f4.rmeta" "--extern" "num_cpus=/home/r/src/rust/miri/test-cargo-miri/target/debug/deps/libnum_cpus-ced524b066799ccf.rmeta" "--extern" "rand=/home/r/src/rust/miri/test-cargo-miri/target/debug/deps/librand-a1a8ab16d61e5ca4.rmeta" "--sysroot" "/home/r/.cache/miri/HOST"

Then I copy-paste that into the shell to run Miri without cargo buffering it. (No env vars should be needed.)

[jonhoo]

Hmm, interesting:

$ cargo +nightly miri test -- --test basic_ref
    Checking flurry v0.3.0 (/home/jon/dev/streams/flurry)
error: Option 'test' given more than once

error: could not compile `flurry`.
$ cargo +nightly miri test -- hasher
    Checking flurry v0.3.0 (/home/jon/dev/streams/flurry)
error: multiple input filenames provided (first two filenames are `src/lib.rs` and `hasher`)

error: could not compile `flurry`.

To learn more, run the command again with --verbose.

I think there's definitely something funky going on there.

I also tried

$ cargo +nightly miri test -- -- --test basic_ref

But that just ran all the test targets (but no tests within each target), making it seem like it was treated as a filtering argument? This seems to be supported by -- -- hasher only running the hasher tests. It seems like miri decides what targets to build/run independently of any arguments, and then just blindly passes all the trailing arguments to every invocation?

[jonhoo]

Your point about how to avoid buffering seems to have worked. I wonder how this normally works when you run cargo test -- --nocapture, because that seems to print partial lines?

[jonhoo]

@RalfJung the hanging tests were (luckily) just due to high iteration counts, which were too slow for miri to get through in a reasonable amount of time. See d0f080b

[RalfJung]

cargo +nightly miri test -- --test basic_ref is passing --test to Miri, so this is the rustc --test flag you are setting -- which as the message says already got said by cargo, this is not what you want.

I don't know which binary you want to pass --test to, cargo, Miri or the test runner itself -- depending on what you want, you need to put it at the right spot between the --. I have never seen that parameter for test runs before.

cargo test -- --nocapture passes --nocapture to the test runner, so the Miri equivalent is cargo miri test -- -- --nocapture.

I wonder how this normally works when you run cargo test -- --nocapture, because that seems to print partial lines?

Then cargo knows it's running a user binary and doesn't do buffering. But with cargo miri test, under the hood the actual command that's being run is cargo build <flags>, and then when cargo thinks it is building the binary, really Miri just runs it. But then cargo still does buffering.

[jonhoo]

--test goes to cargo build, but I couldn't find any place to put it where it did the right thing. For context, --test foo will build and run only tests/foo.rs. --test cannot be passed to rustc, since rustc is just told to compile a particular file. It cannot be passed to the test binary, since at that point it's too late — that test binary has already been compiled.

As for --nocapture, I was more musing as to how it worked to see if miri could do something similar. --nocapture to a miri test binary probably won't have much of an effect since, as you say, the output is buffered by cargo.

[RalfJung]

So cargo test --test foo is a thing? Yeah Miri doesn't support that, I think that would be rust-lang/miri#739.

--nocapture to a miri test binary probably won't have much of an effect since, as you say, the output is buffered by cargo.

Well it still affects whether the test runner itself captures and buffers stdout of the test methods (and suppresses it AFAIK for passing tests).

[jonhoo]

Yup, it's a thing, and a fairly handy one at that! I'll move the discussion to that issue, as it seems exactly appropriate.