-
Notifications
You must be signed in to change notification settings - Fork 352
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
Miri stdout is buffered and stdin in always empty #1505
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Also it doesn't look like servo's ipc-channels have filenames, so I am not sure how we could pass them down to the child process. A unix domain socket in a temporary directory would do it, but the crate only seems to support anonymous channels. Oh and of course this seems like a horrible hack.^^ A better solution might be being able to instruct cargo to just forward stdin and stdout unchanged. That would mean it also gets forwarded to the invoked rustc processes, but that still seems better than the current situation. |
I don't think there is a way to do that in cargo directly. I would probably do things in two phases. Run |
The channels implement Serialize and Deserialize. |
Oh, that's pretty cool actually, as we don't need the separate binary anymore (except for the new RUSTC_WRAPPER, but that's just a "serialize all arguments" binary with no relation to miri) as Another alternative is to have |
I think I'd pefer to keep the separate binary. Currently
That doesn't work when stdin gets more data during execution (think: the program prints a question on stdout and expects a user answer on stdin). |
One concern I have with this data-gathering scheme is concurrency -- there can be multiple binaries to run and we need to capture all their command-line arguments. Is adding a new line to a file atomic? Or should we instead use a folder and create one file per run in that folder? Also, we might have to capture more than just the CLI arguments -- e.g., env vars or the working directory. |
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`
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`
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`
Miri can now read from stdin (thanks to @samrat), but unfortunately stdin is always empty when doing
cargo miri run
. I think this is because of the way that we are contortingcargo
into running Miri -- Miri needs all the compiler flags, so we runcargo check
and setRUSTC_WRAPPER
to ourselves to dispatch "compiling" the final binary to Miri. cargo thinks it is running a compiler, and does not forward any stdin to it. (Here is a test that should work.)Similarly, stdout is always buffered, so even if the program flushes stdout,
cargo miri run
will only print it after the next newline (this is rust-lang/cargo#6641).@ehuss is there any chance that we could get cargo to invoke Miri in a way that's more like
cargo run
and less like running a compiler?The text was updated successfully, but these errors were encountered: