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

Get rid of the test-miri flag in config.toml #61833

Closed
oli-obk opened this issue Jun 14, 2019 · 3 comments · Fixed by #63162
Closed

Get rid of the test-miri flag in config.toml #61833

oli-obk opened this issue Jun 14, 2019 · 3 comments · Fixed by #63162
Labels
A-miri Area: The miri tool T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2019

The test-miri flag changes whether -Zalways-encode-mir is passed to rustc, causing all metadata beyond stage 0 to include MIR of all functions, even private monomorphic ones. Instead, what we can do is to have miri tests first run cargo miri setup to generate that special libstd in CI in addition to the regular libstd. Then tests can be run with special libstd.

cc @RalfJung

@oli-obk oli-obk added T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. A-miri Area: The miri tool labels Jun 14, 2019
@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2019

Worse, it also passes --cfg miri to rustc, meaning we get the hashbrown without SIMD!

@RalfJung
Copy link
Member

RalfJung commented Jun 14, 2019

Oh also, ideally this could use or at least share some code with the build script Miri uses to do the same on its own CI.

EDIT: Or maybe not, that does some crazy RUSTFLAGS stuff that probably should not be done for the released artifact.

@RalfJung
Copy link
Member

I submitted a PR for this at #63162.

Centril added a commit to Centril/rust that referenced this issue Aug 8, 2019
Miri tests: use xargo to build separate libstd

This uses `cargo miri setup` to prepare the libstd that is used for testing Miri, instead of adjusting the entire bootstrap process to make sure the libstd that already gets built is fit for Miri.

The issue with our current approach is that with `test-miri = true`, libstd and the test suite get built with `--cfg miri`, which e.g. means hashbrown uses no SIMD, and not all things are tested. Such global side-effects seem like footguns waiting to go off.

On the other hand, the new approach means we install xargo as a side-effect of doing `./x.py test src/tools/miri`, which might be surprising, and we also both have to build xargo and another libstd which costs some extra time. Not sure if the tools builders have enough time budget for that. Maybe there is a way to cache xargo?

We have to first first land rust-lang/miri#870 in Miri and then update this PR to include that change (also to get CI to test Miri before bors), but I wanted to get the review started here.

Cc @oli-obk (for Miri) @alexcrichton (for CI) @Mark-Simulacrum (for bootstrap)

Fixes rust-lang#61833, fixes rust-lang#63219
Centril added a commit to Centril/rust that referenced this issue Aug 8, 2019
Miri tests: use xargo to build separate libstd

This uses `cargo miri setup` to prepare the libstd that is used for testing Miri, instead of adjusting the entire bootstrap process to make sure the libstd that already gets built is fit for Miri.

The issue with our current approach is that with `test-miri = true`, libstd and the test suite get built with `--cfg miri`, which e.g. means hashbrown uses no SIMD, and not all things are tested. Such global side-effects seem like footguns waiting to go off.

On the other hand, the new approach means we install xargo as a side-effect of doing `./x.py test src/tools/miri`, which might be surprising, and we also both have to build xargo and another libstd which costs some extra time. Not sure if the tools builders have enough time budget for that. Maybe there is a way to cache xargo?

We have to first first land rust-lang/miri#870 in Miri and then update this PR to include that change (also to get CI to test Miri before bors), but I wanted to get the review started here.

Cc @oli-obk (for Miri) @alexcrichton (for CI) @Mark-Simulacrum (for bootstrap)

Fixes rust-lang#61833, fixes rust-lang#63219
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants