rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.31k stars 327 forks source link

Handle Miri sysroot entirely outside the Miri driver #3411

Closed RalfJung closed 4 months ago

RalfJung commented 5 months ago

(Extracted from https://github.com/rust-lang/miri/pull/3409)

This entirely moves the responsibility of setting miri-sysroot to whatever invokes the Miri driver. cargo-miri knows whether it is inside rustdoc or not and can adjust accordingly. I previously avoided doing that because there are a bunch of places that are invoking the driver (cargo-miri, the ui test suite, ./miri run, ./x.py run miri) and they all need to be adjusted now. But it is also somewhat less fragile as we usually have more information there -- and we can just decide that ./miri run file.rs --sysroot path is not supported. The advantage of this is that the driver is reasonably clean and doesn't need magic environment variables like MIRI_SYSROOT, and we don't have to fight rustc_driver to use a different default sysroot. Everything is done in cargo-miri (and the other much simpler driver wrappers) where it can hopefully be debugged much better.

RalfJung commented 5 months ago

The issue with this is that it breaks autocfg for cross-target interpretation. We set RUSTC to be the Miri driver, and autocfg ignores RUSTC_WRAPPER, so it directly invokes the driver and there is no way for us to inject the right sysroot.

So this is probably blocked on https://github.com/cuviper/autocfg/issues/26. (dtolnay's crates seem to already support RUSTC_WRAPPER.)

bors commented 5 months ago

:umbrella: The latest upstream changes (presumably #3414) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung commented 5 months ago

autocfg got patched. :) So this should pass CI now.

It's still a bit fragile though due to https://github.com/rust-lang/cargo/issues/10885. Also if we land this now cross-builds will break for anyone using autocfg older than 1.2. (But host builds should work, autocfg probes will just use the wrong sysroot.)

RalfJung commented 5 months ago

Ah the eyre build probe also still fails to apply RUSTC_WRAPPER. So landing this would break everything that uses eyre...

IOW, blocked on https://github.com/eyre-rs/eyre/issues/84.

bors commented 5 months ago

:umbrella: The latest upstream changes (presumably #3418) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung commented 5 months ago

Actually looks like eyre still works with this. I guess by using neither RUSTC_WRAPPER nor --target it ends up checking the wrong thing (it checks whether the build succeeds for the host, not the target we are actually building for) but the check succeeds. I hope eyre doesn't check for anything target-specific in its build probe. Or maybe the check fails but it is done in a way that failure is harmless (nightly feature gets deactivated and then everything works fine).

The one thing that does not work any more with this PR is setting --target but then not applying RUSTC_WRAPPER. Only autocfg ever did that. (And some old versions of anyhow/thiserror, but that has been fixed long ago.)

So I think this is currently my preferred approach to getting rid of the --sysroot command-line hackery in the Miri driver. @rust-lang/miri how long do you think we should wait before rolling this out? autocfg 1.2 is a semver-compatible update but it has been released very recently, so it will take a bit until everyone picks it up.

RalfJung commented 5 months ago

Also this should only break with old autocfg when using --target; plain cargo miri test should be unaffected.

saethlin commented 5 months ago

how long do you think we should wait before rolling this out?

Based on the downloads graph on crates.io, 1.2 seems like just over a third of downloads. I'm concerned we have a surprising amount of --target usage due to our much better shim support on x86_64-unknown-linux-gnu, so I would prefer to wait a week from now.

bors commented 5 months ago

:umbrella: The latest upstream changes (presumably #3453) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung commented 4 months ago

I would prefer to wait a week from now.

We have waited 2 weeks since then. Download stats for autocfg haven't really changed.

Let's ship this; we can always revert if there are too many problems. @bors r+

bors commented 4 months ago

:pushpin: Commit 8727602106f3b7df3460af8b678cadf77cf16a1c has been approved by RalfJung

It is now in the queue for this repository.

bors commented 4 months ago

:hourglass: Testing commit 8727602106f3b7df3460af8b678cadf77cf16a1c with merge f42ede2f19693d2b23e94536250b0f4b9416e607...

RalfJung commented 4 months ago

The build has long finished, but somehow bors didn't notice...? @bors retry

bors commented 4 months ago

:hourglass: Testing commit 8727602106f3b7df3460af8b678cadf77cf16a1c with merge f250781a1397517aa19d87ffad07ac2234b6f7dd...

bors commented 4 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing f250781a1397517aa19d87ffad07ac2234b6f7dd to master...