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

cargo miri test does not run doc-tests #584

Closed
RalfJung opened this issue Dec 21, 2018 · 35 comments · Fixed by #1757
Closed

cargo miri test does not run doc-tests #584

RalfJung opened this issue Dec 21, 2018 · 35 comments · Fixed by #1757
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

I am not sure what it would take to also make it build and execute doc-tests. Probably just some tweaks in how we wrap cargo rustc?

@bjorn3
Copy link
Member

bjorn3 commented Dec 21, 2018

Doc tests need rustdoc as driver, while miri needs itself as driver.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-cargo Area: affects the cargo wrapper (cargo miri) labels Mar 8, 2019
@SimonSapin
Copy link

@rust-lang/rustdoc, can rustdoc be used as a library (not a driver) to extract the doctests without running them?

@RalfJung
Copy link
Member Author

RalfJung commented Jul 16, 2019

Or, alternatively, can we set some environment variable to make rustdoc call miri instead of calling rustc and running the binary on each test after extraction?

@QuietMisdreavus
Copy link
Member

QuietMisdreavus commented Jul 16, 2019

@rust-lang/rustdoc, can rustdoc be used as a library (not a driver) to extract the doctests without running them?

Unfortunately, librustdoc was taken out of the sysroot a while back, and i don't think we auto-publish the source for that as a crate either, so there's not really an easy solution for that. Rustdoc itself doesn't really provide functionality to only save off the extracted/converted doctest, either.

Or, alternatively, can we set some environment variable to make rustdoc call miri instead of calling rustc and running the binary on each test after extraction?

Same response - unless the test is manually marked as ignore (or given some non-rust language string) in the source, rustdoc will always run the converted doctest through rustc via the librustc_interface interface. There's an open PR to provide an option to run the compiled binaries through a "test runner", but it doesn't sound like that will help either.

@RalfJung
Copy link
Member Author

How open would the rustdoc team be to provide a way for Miri to run the extracted tests, and what would be the preferred way of achieving that (librustdoc vs controlling what happens after extraction)?

For me it seems like the second option is the simpler one.

@bjorn3
Copy link
Member

bjorn3 commented Jul 16, 2019

@RalfJung
Copy link
Member Author

Yeah, now I see what @QuietMisdreavus meant by librustc_interface -- it does not call the rustc binary, it calls the rustc library.

The "easiest" way to get Miri into this would be to provide a way to call some binary instead, but I am not sure if that is feasible.

@QuietMisdreavus
Copy link
Member

If there were some flag we could provide to just save off all the doctests as standalone rust files, would that be enough to allow miri to attempt to build them? They're meant to be compiled as binaries and link against the regular crate and its dependency tree.

@RalfJung
Copy link
Member Author

If there were some flag we could provide to just save off all the doctests as standalone rust files, would that be enough to allow miri to attempt to build them?

I guess we could make cargo miri test pick them up and run them all through Miri. The hard part would be to figure out the right command-line flags to get all the dependencies to work, but I am not sure if rustdoc can even help there?

@QuietMisdreavus
Copy link
Member

Cargo hands rustdoc all the --extern flags it needs when running rustdoc --test from cargo test, so presumably that information would be present in the crate's metadata.

@RalfJung
Copy link
Member Author

Hm, that raises the interesting question how cargo miri test would invoke rustdoc. Probably through cargo test? Can we do that targeting just the doctests of a particular binary?

Currently I wouldn't know how cargo miri test can get those flags; for the binaries we run we get them from cargo as well (cargo miri sets itself into the RUSTC env var so that cargo calls it with all the right flags and it just has to forward them).

@nox
Copy link

nox commented Aug 2, 2019

Does rustdoc reads RUSTC? If so, could we make miri compile a small executable that runs the test under miri indirectly so that the changes to rustdoc are minimal?

@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2019

AFAIK rustdoc uses the rustc driver interface directly, it does not run a binary.

@nox
Copy link

nox commented Aug 2, 2019

Oh right, I actually read that earlier in the thread today, but forgot about it now that it's Friday evening… My bad.

@RalfJung
Copy link
Member Author

With rust-lang/rust#63827, rustdoc now runs rustc out-of-process. That should make it easier to run Miri instead.

@andjo403 is there a way to use an env var or so to control the binary that is run?

@andjo403
Copy link

not yet working on a new PR where RUSTC env var can be set but have some test to fix

@andjo403
Copy link

@Mark-Simulacrum added the --test-builder option in rust-lang/rust#64328 for using other binary and it is merged now

@WaffleLapkin
Copy link
Member

Is it possible now to somehow trick miri to run doctests?

@RalfJung
Copy link
Member Author

rustdoc probably lets us do that these days, but things have not been wired up in Miri yet.

@RalfJung RalfJung mentioned this issue Sep 8, 2020
10 tasks
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
bors added a commit that referenced this issue Sep 12, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. ~~I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.~~ (postponed that until we have a concrete example)
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
bors added a commit that referenced this issue Sep 17, 2020
Redo cargo-miri logic

This rewrite the cargo-miri logic for running the requested crate(s) following what we outlined in #739: `cargo miri run/test $FLAGS` (almost) literally invokes `cargo run/test $FLAGS` but with some environment variables set so that we can control what happens:
* `RUSTC_WRAPPER` is set so that we get invoked instead of `rustc`. We use that power to mess with the flags being used for the build (things to be interpreted by Miri use a different sysroot), and when we are detecting a binary crate meant to be run by Miri, we grab the info we care about and put it into a JSON file for later use.
* `CARGO_TARGET_$TARGET_RUNNER` is set so what we get invoked when cargo wants to run a binary. At that point we take that JSON info from before and use it to invoke Miri.

Overall this works great! We get all sorts of cargo magic for free, and do not even need `cargo-metadata` any more. There's one annoying point though, which I have not managed to entirely work around yet: this means we are doing a full build, not just a check-build. Given that our sysroot is MIR-only, I was surprised that this even worked, but still -- this means we are doing more work than we should. So I also added some patching of arguments to make sure `--emit` does not contain `link`, and then more patching was required of the `--extern` flags for the binary because those referenced the `.rlib` files but now only `.rmeta` exists, and that is still not fully working because cargo seems to expect those `.rmeta` files and now triggers a rebuild each time as those files are still missing. My current plan is to make our wrapper create some empty dummy files with the right names, but the amount of hacks we are stacking on top of each other here is getting worrysome.^^ `@ehuss` your input would be welcome on this issue.

Due to re-using cargo more literally, this also changes flag parsing to match `cargo`. So `-Zmiri` flags now have to be passed via an environment variable (Cc #1416).

This PR is not ready yet, but the parts that are there I think can be reviewed already. TODO:
* [x] [Fix Windows](#1540 (comment)).
* [x] Figure out how we can do check-only builds without the rebuild problem above. ~~I am also worried about cases where `build.rs` script might detect check-only builds and then do less work; I think some crates in rustc are doing that and if this is a thing in the wider ecosystem we need to find a way to support this as well.~~ (postponed that until we have a concrete example)
* [x] Currently cargo runs doctests as well, and they are not run in Miri. We should at least figure out a way to not run them at all (resolving #584 is left for a future PR).
* [x] For some time, detect the old way of passing `-Zmiri` flags and warn that this will need updating. For some simple situations we can probably make it still support the old way, but I plan to remove those hacks after a bit. This is just to give people and me time to go around and send PRs to all projects that use Miri on CI, and update their use of the flags.
* [x] Add a test for stdin handling (#1505). This should work now but we should be sure.
* [x] Add a test for cargo env vars (#1515).
* [x] Check if #1516 is resolved.
* [x] Check if #1001 and #1514 are resolved.
* [x] Check if #1312 is resolved (not sure if it is wort adding a test).
* [x] Check if #1512 is resolved (not sure if it is wort adding a test).

Fixes #700.
Fixes #739.
Fixes #1001.
Fixes #1312 (without a test, as we run without cargo's stdin/stdout wrapping now, as the test for stdin confirms).
Fixes #1416.
Fixes #1505.
Fixes #1512 (without a test, as cargo now does all that handling anyway, which various other tests confirm).
Fixes #1514.
Fixes #1516.

Cc `@alecmocatta` who reported many of the bugs above; would be great if you could help with the tests e.g. by providing some small examples I could try.
r? `@oli-obk`
@teryror
Copy link
Contributor

teryror commented Jan 10, 2021

Is anyone actively working on this?

I'd be willing to invest a couple of afternoons over the coming week or two, but I could definitely use some pointers on how to get started. I never touched any of the involved projects before. I did read the contributor's guide, though.

Based on what's been said here and what I gathered from reading code, this would basically boil down to replacing this error message with a call to a new function phase_cargo_rustdoc, pretty much without changing anything else. Is that assessment correct?

The problem is I'm not clear on what that function should do exactly. Invoke rustdoc --test --test-builder {?}, then the executable that produces, presumably?

@RalfJung
Copy link
Member Author

Is anyone actively working on this?

No, not that I know of.

I'd be willing to invest a couple of afternoons over the coming week or two, but I could definitely use some pointers on how to get started. I never touched any of the involved projects before. I did read the contributor's guide, though.

That's great. :) However, I'm afraid there's not a ton of guidance available on this issue -- part of the problem here is even figuring out how to best do this.

Based on what's been said here and what I gathered from reading code, this would basically boil down to replacing this error message with a call to a new function phase_cargo_rustdoc, pretty much without changing anything else. Is that assessment correct?

Yes, that sounds right.

The problem is I'm not clear on what that function should do exactly. Invoke rustdoc --test --test-builder {?}, then the executable that produces, presumably?

I don't know either what exactly it should do. ;) Probably the first step is to figure out the interaction between rustdoc and cargo, which is the thing that we are "hooking" here. Does cargo call rustdoc to build some binaries, and then the binaries are run by cargo, or does cargo run rustdoc and rustdoc runs some binaries? What kind of command-line arguments does cargo pass to rustdoc, and does Miri have to patch them in any way (besides adding --test-builder)? There will be some exploration and research needed to figure this out, and indeed that exploration and research is the largest part of the work that needs to be done to resolve this issue.

@teryror
Copy link
Contributor

teryror commented Jan 10, 2021

Alright. I've skimmed what seemed to be the relevant parts of cargo, I guess I'll have to take a closer look then.

Thanks for the guiding questions, that's already pretty helpful!

@RalfJung
Copy link
Member Author

Awesome. :) Don't hesitate to ask if you have further questions. (But also, I might not always be able to answer so quickly, so when delays happen that's just because I am busy.)

Judging from --test-builder, I think rustdoc is only building binaries, not running them. So likely Miri should make rustdoc invoke miri as the builder and make sure that this rustdoc-invoked Miri enters phase_cargo_rustc. Assuming this is always a binary crate, this would make Miri emit a JSON file as the "binary" with all the required information that is needed later when cargo tells Miri to "run" that JSON file.

@teryror
Copy link
Contributor

teryror commented Jan 11, 2021

Status report!

I spent some time reading the source to librustdoc, and it looks to me like you were mistaken. After spawning a test_builder process to produce an output_file, rustdoc tries to execute it. However, it also has the --runtool and --runtool-arg options. If a run tool is provided, it invokes

$runtool ($runtool-arg)* $output_file

instead. The run tool should be the driver miri, rather than cargo-miri, correct? Under that assumption, here's a naive draft:

I can't test this quite yet, because the --test-builder option appears to be broken. I'll try forking the rust repo later today so I might give this a shot. I was still hoping I wouldn't have to do that, but c'est la vie.

EDIT: Ok, I have a local build of rustdoc that I can actually hook into. Turns out my naive draft didn't work, no big surprise there.

The way rustdoc invokes the test builder is indistinguishable from the way cargo invokes rustdoc, at least from the command line arguments. It does, however, set some UNSTABLE_ environment variables we can check for. Not a great thing to depend on, but it'll do, I suppose. The issue I'm facing now is that rustdoc passes the generated code to rustc via stdin, rather than a temporary file. I can't think of a simple way to trick cargo-miri into just dealing with that.

Does the miri driver support piping code in like that? If so, I could make phase_cargo_rustc record the contents of stdin in the temporary JSON file, and make phase_cargo_runner play them back, so to speak. Alternatively, I could add two more phases, phase_rustdoc_rustc and phase_rustdoc_runner, respectively. Thoughts, @RalfJung?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 12, 2021

I spent some time reading the source to librustdoc, and it looks to me like you were mistaken. After spawning a test_builder process to produce an output_file, rustdoc tries to execute it. However, it also has the --runtool and --runtool-arg options.

Ah, interesting -- good find.
This will mean more work for you.^^ Yes, the runtool will have to be Miri, but it will need all the arguments that were passed to --test-builder. That's the same issue as with regular cargo builds and running them; it's why the builds create JSON files -- then when running them we have all the aguments available that were passed for the build.

The way rustdoc invokes the test builder is indistinguishable from the way cargo invokes rustdoc, at least from the command line arguments. It does, however, set some UNSTABLE_ environment variables we can check for. Not a great thing to depend on, but it'll do, I suppose.

Well, phase_cargo_rustdoc could set an env var, wouldn't that help?

The issue I'm facing now is that rustdoc passes the generated code to rustc via stdin, rather than a temporary file. I can't think of a simple way to trick cargo-miri into just dealing with that.

Ugh. Yes that will be interesting.^^

Does the miri driver support piping code in like that? If so, I could make phase_cargo_rustc record the contents of stdin in the temporary JSON file, and make phase_cargo_runner play them back, so to speak. Alternatively, I could add two more phases, phase_rustdoc_rustc and phase_rustdoc_runner, respectively.

Miri uses rustc for input file handling, so stdin should just work. It has never been tested though.
I'd prefer reusing existing code as much as possible, i.e., phase_cargo_rustc and phase_cargo_runner should coordinate to forward stdin when necessary. The tricky bit will be to not do this always but only when needed; in the "normal" case we'll still want to use the normal stdin for running Miri so that the interpreted program can read from stdin. I guess this could either be smart and figure things out based on the rustc arguments, or phase_cargo_rustdoc could set an env var which says that stdin should be handled this way.

@bjorn3
Copy link
Member

bjorn3 commented Jan 12, 2021

Ugh. Yes that will be interesting.^^

It also sets a few env vars to fixup the source location shown in error messages.

@teryror
Copy link
Contributor

teryror commented Jan 12, 2021

It also sets a few env vars to fixup the source location shown in error messages.

You mean UNSTABLE_RUSTDOC_TEST_PATH and UNSTABLE_RUSTDOC_TEST_LINE? That's what I'm checking for :)

Well, phase_cargo_rustdoc could set an env var, wouldn't that help?

That would probably help in orchestrating the other phases, but I'm testing the UNSTABLE_* vars to see if I should call phase_cargo_rustdoc in the first place.

Say if MIRI_CALLED_FROM_RUSTDOC is set, phase_cargo_rustc reads all of stdin and dumps it into the JSON file, and phase_cargo_runner just needs to know about the optional field. Seems like a decent plan, no?

I'll give that a shot later today, but first I'll have to add a regression test to rustdoc to get my PR accepted :)

@RalfJung
Copy link
Member Author

That would probably help in orchestrating the other phases, but I'm testing the UNSTABLE_* vars to see if I should call phase_cargo_rustdoc in the first place.

Why is that needed? This case distinction already exists in master (to emit the error message).

Say if MIRI_CALLED_FROM_RUSTDOC is set, phase_cargo_rustc reads all of stdin and dumps it into the JSON file, and phase_cargo_runner just needs to know about the optional field. Seems like a decent plan, no?

Yes that works.

@teryror
Copy link
Contributor

teryror commented Jan 12, 2021

Why is that needed? This case distinction already exists in master (to emit the error message).

It's not, you're right, my mistake. I got confused for a second there. This should work perfectly fine then.

@teryror
Copy link
Contributor

teryror commented Jan 12, 2021

I seem to have a mostly working prototype! See the full code here and the diff here.

MIRI_VERBOSE output is swallowed for passing tests, of course, but purposefully failing one shows that phase_cargo_rustc and phase_cargo_runner are indeed called appropriately, and the test output matches my expectation. Miri seems to be dealing with piped input just fine.

Problem is that compile_fail tests all fail due to being erroneously compiled without error, I'm not sure why that is yet. Actually, I know what the problem is: I'm not piping the test code into that Miri invocation yet! I'll still call it a day for now, though.

EDIT: Ugh, it's a bit more complicated than that. The issue is the early return after writing the JSON file in phase_cargo_rustc. No checking takes place there, but rustdoc expects the test-builder process to exit with an error code for a compile_fail test, in which case the runtool is never invoked. I could either add some special case logic there or try to rewire a good chunk of the function without breaking anything.

EDIT 2: I went with a special case, for now. I figure it's easier to refactor later once I have a solution figured out.

I can't seem to get the check-only build to work though, Miri keeps complaining about not being able to find the library crate the doc tests came from. I think that's because cargo-miri creates stub *.rlib files for that, but I'm not sure that's the issue, and even if it is, I have no idea what to do about it. I'm already patching the --emit arguments like the existing code does (removing link and adding metadata), and I tried patching the --extern arguments to use *.rmeta instead (like the test runner, which doesn't have any issues finding the crate), but none of that helped. I'm out of ideas here, so uh... suggestions?

@RalfJung? (not sure if you see the edits if I don't mention you)

EDIT 3: Okay, patching the --extern arguments in phase_cargo_rustdoc seems to get me somewhere. Normal tests still work, if I comment out the check-only build in phase_cargo_rustc, but leaving that enabled now causes an ICE instead. Just one example from my library coca:

[cargo-miri rustc] "/home/tld/.cargo/bin/miri" "--sysroot" "/home/tld/.cache/miri/HOST" "--crate-type" "bin"
"--cfg" "feature=\"default\"" "--edition" "2018" "-o" "/tmp/rustdoctestnmsoFZ/rust_out"
"-L" "dependency=/home/tld/Documents/coca/target/x86_64-unknown-linux-gnu/debug/deps"
"-L" "dependency=/home/tld/Documents/coca/target/debug/deps"
"--extern" "coca=/home/tld/Documents/coca/target/x86_64-unknown-linux-gnu/debug/deps/libcoca-8671386d903c290f.rmeta"
"--extern" "rand=/home/tld/Documents/coca/target/x86_64-unknown-linux-gnu/debug/deps/librand-af6e186424c4447e.rmeta"
"-Ccodegen-units=1" "-C" "embed-bitcode=no" "-Z" "unstable-options" "--target" "x86_64-unknown-linux-gnu" "--color" "always" "-"
error: internal compiler error: compiler/rustc_mir/src/monomorphize/collector.rs:826:9:
cannot create local mono-item for DefId(2:6795 ~ core[1258]::fmt::num::DEC_DIGITS_LUT)

thread 'rustc' panicked at 'Box<Any>', compiler/rustc_errors/src/lib.rs:958:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.51.0-nightly (7bb163095 2021-01-14) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z unstable-options -C codegen-units=1 -C embed-bitcode=no --crate-type bin

query stack during panic:
#0 [collect_and_partition_mono_items] collect_and_partition_mono_items
end of query stack
error: aborting due to previous error

They all hit this exact same error, down to the DefId(2:6795 ~ core[1258]::fmt::num::DEC_DIGITS_LUT). I don't see how this could be related to the specific code the command is fed, but I'll have to investigate what's actually causing this.

Now that I think about it, this might just be because I switched from my local rustc/rustdoc build to the newest nightly, and Miri just doesn't work with that yet.

@teryror
Copy link
Contributor

teryror commented Jan 14, 2021

I got it to work! 🎉

I was missing an --emit=dep-info,metadata argument to the check-only build. The ICE still should be considered a bug, though, right?

I went ahead and opened a PR :D

@RalfJung
Copy link
Member Author

That's awesome. :) Thanks a lot for your work @teryror!

@cmazakas
Copy link

I'm having trouble getting Miri to ignore certain doctests. Namely, I have some doctests that use SIMD.

  /// # Example
  /// ```
  /// #[cfg_attr(miri, ignore)]
  /// fn main() {

The above doctest winds up getting run anyway. Is there any method used to ignore a doctest?

@RalfJung
Copy link
Member Author

I don't think ignore does anything for doctests.

Did you try #[cfg(not(miri))]?

@cmazakas
Copy link

Ah, perfect! That works, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo Area: affects the cargo wrapper (cargo miri) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants