Support running library tests in Miri

[RalfJung]

This adds a new bootstrap subcommand ./x.py miri which can test libraries in Miri. This is in preparation for eventually doing that as part of bors CI, but this PR only adds the infrastructure, and doesn't enable it yet.

@rust-lang/bootstrap should this be x.py test --miri library/core or x.py miri library/core? The flag has the advantage that we don't have to copy all the arguments from Subcommand::Test. It has the disadvantage that most test steps just ignore --miri and still run tests the regular way. For clippy you went the route of making it a separate subcommand. I went with a flag now as that seemed easier, but I can change this. I made it a new subcommand. Note however that the regular cargo invocation would be cargo miri test ..., so x.py is still going to be different in that the test is omitted. That said, we could also make it ./x.py miri-test to make that difference smaller -- that's in fact more consistent with the internal name of the command when bootstrap invokes cargo.

@rust-lang/libs unfortunately this PR does some unholy things to the lib.rs files of our library crates.
@m-ou-se found a way that entirely avoids library-level hacks, except for some new small lib.miri.rs files that hopefully you will never have to touch. There's a new hack in cargo-miri but there it is in good company...

[rustbot]

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[m-ou-se]

This adds a --miri flag to ./x.py test which will cause library tests to be run in Miri.

Nice! Thank you so much for working on this!

unfortunately this PR does some unholy things to the lib.rs files of our library crates.

Oh. :(

I hope this is okay because the crate root files are not changed that often.

My concerns:

• All library maintainers and contributors (at least those who need to do things in the root of the crates) will need to understand which items need which cfg attribute. After looking at this for ten seconds, it's not obvious to me when I should apply #[cfg(not(all(feature = "miri-test", not(any(test, doctest)))))] to something or even what that cfg means in practice. Maybe it'd be clearer if I took a bit more time, but some clear guideline somewhere would be helpful.

• Putting most things in lib_.rs while having some parts in lib.rs will result in having to jump even more between files, increasing the feeling of 'being lost' that many library contributors already often run into.

• In general, the closer we can keep these crates to how a 'normal' crate would look like, the better. Any added 'weirdness' will make it harder to contribute and thus for new contributors to get started.

• Does rust-analyzer handle this perfectly? We also have some tools that try to parse these crates with syn that will probably break. (Although I believe those simple tools have already been broken for a while, for other reasons.)

These aren't strict requirements; it's always a trade-off of course. Being able to run miri from x.py is also very valuable. But I think it's definitely worth it for us to spend a bit more time trying to think of a solution that introduces less weirdness for library contributors/maintainers.

I see you opened a topic for this on Zulip. Let's use that for a bit of brainstorming on alternatives. Maybe someone can come up with a good idea.

[RalfJung]

All library maintainers and contributors (at least those who need to do things in the root of the crates) will need to understand which items need which cfg attribute. After looking at this for ten seconds, it's not obvious to me when I should apply #[cfg(not(all(feature = "miri-test", not(any(test, doctest)))))] to something or even what that cfg means in practice. Maybe it'd be clearer if I took a bit more time, but some clear guideline somewhere would be helpful.

Everything should have #[cfg(not(all(feature = "miri-test", not(any(test, doctest)))))], except for the "real{core,alloc,std}" re-export that should never be changed. I did add comments explaining this as clearly as I could; happy to improve those comments if you have suggestions:

// We want to be able to `cargo miri test` this crate, but that's tricky:
// This will build the crate itself again, so now we have two `core` crates, one in the sysroot and
// one built as dependency of the test crate. That leads to duplicate lang item errors. `./x.py test
// library` avoids this by making the sysroot exactly the crates that `cargo build` produces, but
// Miri builds the sysroot itself so that does not work.
// Instead, we use the `miri-test` feature to indicate that we're running `cargo miri test`, and
// with that feature we turn this crate into a re-export of the sysroot crate. We only do this when
// building the crate that will become a dependency, not when doing the actual (doc)test build.

[...]

// Otherwise, we build the crate as usual. To avoid having to repeat the same `cfg` so many times,
// we `include!("lib_.rs")` with the main crate contents, and `cfg` the `include!`. However, some
// macro-related things can't be behind the `include!` so we have to repeat the `cfg` for them.

In guess so far these are more focused on explaining what happens, than to giving guidance for what to do when things change. I have pushed a small extension of the comments that should help.

Putting most things in lib_.rs while having some parts in lib.rs will result in having to jump even more between files, increasing the feeling of 'being lost' that many library contributors already often run into.

We can also add the cfg to every single mod or use in the crate root. But to me that seemed worse.

Does rust-analyzer handle this perfectly?

It's hard to say whether it handles this "perfectly". But so far I have not noticed any issues. It seems to support include! just fine.

[Mark-Simulacrum]

I'm broadly in favor of x.py miri vs a dedicated flag. It's not quite cargo miri test, as you point out, but the general idea is to match the Cargo interface users see externally as much as possible. It also feels potentially a bit more discoverable? (Unclear).

[RalfJung]

Oh, ./x.py check does not seem to check the bootstrap tests...

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[RalfJung]

Okay that seems to have worked. :)
@bors r=m-ou-se,saethlin,onur-ozkan

[RalfJung]

@bors r=m-ou-se,saethlin,onur-ozkan

[bors]

📌 Commit d953560 has been approved by m-ou-se,saethlin,onur-ozkan

It is now in the queue for this repository.

[RalfJung]

@bors r-
uh, github didn't notice my push?

[RalfJung]

@bors r=m-ou-se,saethlin,onur-ozkan

[bors]

📌 Commit 3ad9c83 has been approved by m-ou-se,saethlin,onur-ozkan

It is now in the queue for this repository.

[RalfJung]

@bors rollup=iffy
as we have failed one before

[bors]

⌛ Testing commit 3ad9c83 with merge 5958f5e...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se,saethlin,onur-ozkan
Pushing 5958f5e to master...

[rust-timer]

Finished benchmarking commit (5958f5e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.208s -> 668.884s (-0.05%)
Artifact size: 318.05 MiB -> 318.05 MiB (-0.00%)

[m-ou-se]

@RalfJung Thanks for making this happen!

[RalfJung]

Thanks for the help. :)