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

Make miri a subtree instead of a submodule #102028

Merged
merged 5,441 commits into from
Sep 22, 2022
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 19, 2022

r? @RalfJung

fixes #101867
fixes #100134

bors and others added 30 commits July 27, 2022 00:39
When cargo-miri is executed as a cargo test runner or rustdoc runtool,
external tools expect what they launch as the runner/runtool to be the
process actually running the test. But in the implementation, we launch
the Miri interpreter as a subprocess using std::process::Command. This
tends to confuse other tools (like nextest) and users (like the author).
What we really want is to call POSIX exec so that the cargo-miri process
becomes the interpreter.

So this implements just that; we call execve via a cfg(unix) extension
trait. Windows has no such mechanism, but it also doesn't have POSIX
signals, which is the primary tripping hazard this change fixes.
Use real exec on cfg(unix) targets

Closes rust-lang/miri#2421

The standard library has a platform extension trait that lets us get the behavior we want on cfg(unix), so why not use it?

I tried this out and it produces the correct behavior in concert with nextest.
Use cargo_metadata in cargo-miri

Closes rust-lang#2393

Added `cargo_metadata` to `cargo-miri` and changed metadata from manual parsing to `cargo_metadata` invocations. Thus, removed local `Metadata` struct too.

Happy to fix if anything isn't right :)
Fix typo in eval.rs

I just found some typos while reading the code
Add shim for realpath on unix

Salvaged from rust-lang/miri#2294 by `@LegNeato`
Co-authored-by: Ralf Jung <post@ralfj.de>
also greatly extend the 'who calls who' comment
split cargo-miri into multiple files

also greatly extend the 'who calls who' comment
…ture

Some crates are using nightly and failing when mapping these errors,
for example <https://miri.saethlin.dev/?crate=remove_dir_all&version=0.7.0>:

```
error: unsupported operation: io error NotADirectory cannot be translated into a raw os error
    --> /root/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/unix/fs.rs:1203:19
```
Add additional raw error mappings for the nightly `io_error_more` feature

Some crates are using nightly and failing when mapping these errors,
for example <https://miri.saethlin.dev/?crate=remove_dir_all&version=0.7.0>:

```
error: unsupported operation: io error NotADirectory cannot be translated into a raw os error
    --> /root/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/unix/fs.rs:1203:19
```
avoid strerror_r failure on unknown errnum

This is an informative function anyway, so as fallback just return a string with the raw errnum. Avoids panics / interpreter aborts in std on unknown errnum in from_raw_os_error.
and a bit of cleanup
add support for env::home_dir

and a bit of cleanup
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 21, 2022
@RalfJung
Copy link
Member

uh...?

 Documenting miri v0.1.0 (/checkout/src/tools/miri)
error[E0275]: overflow evaluating the requirement `rustc_ast::token::TokenKind: std::marker::Send`
[...]
error: could not document `miri`

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 21, 2022

Lol have we ever tried documenting miri?

@saethlin
Copy link
Member

saethlin commented Sep 21, 2022

Yes, when Miri builds its docs have been available on https://doc.rust-lang.org/nightly-rustc

Which is a relatively recent thing, I'm told. The docs did build a week or so ago.

@RalfJung
Copy link
Member

Yeah the docs currently work (well they did before the most recent toolstate breakage).

We can revert #98764 for now if it helps.

@jyn514
Copy link
Member

jyn514 commented Sep 21, 2022

I expect raising the type length limit like it suggests would work fine too.

@rust-log-analyzer

This comment was marked as resolved.

@RalfJung
Copy link
Member

(that's a very old log, the bot just a long time before posting it)

@RalfJung
Copy link
Member

I expect raising the type length limit like it suggests would work fine too.

But why would this start being a problem now?

@oli-obk should we do a regular submodule update first, both to unblock the distributed Miri and to make sure that this PR is a no-op for the Miri folder (so all build failures must definitely be due to the subtree switch)?

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 22, 2022

I just disabled the (newly enabled in this PR) check that docs actually build and we can fix them later

@bors r+

@bors
Copy link
Contributor

bors commented Sep 22, 2022

📌 Commit 2ce88a5 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 22, 2022

But yes, if this one doesn't go through, let's do a submodule bump, I have enough experience manually "rebasing" (recreating) this PR now for that not to be annoying

@bors
Copy link
Contributor

bors commented Sep 22, 2022

⌛ Testing commit 2ce88a5 with merge c10f7d7...

@bors
Copy link
Contributor

bors commented Sep 22, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c10f7d7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2022
@bors bors merged commit c10f7d7 into rust-lang:master Sep 22, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 22, 2022
@bors bors mentioned this pull request Sep 22, 2022
3 tasks
@oli-obk oli-obk deleted the miri_subtree branch September 22, 2022 12:40
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c10f7d7): 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.

mean1 range count2
Regressions ❌
(primary)
0.6% [0.6%, 0.6%] 1
Regressions ❌
(secondary)
3.7% [2.6%, 4.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.5% [-6.5%, -2.5%] 2
All ❌✅ (primary) 0.6% [0.6%, 0.6%] 1

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-2.7%, -2.2%] 2
Improvements ✅
(secondary)
-7.2% [-7.2%, -7.2%] 1
All ❌✅ (primary) -2.4% [-2.7%, -2.2%] 2

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@jyn514
Copy link
Member

jyn514 commented Sep 22, 2022

@RalfJung @oli-obk would one of you mind making a follow-up PR to address the comments I had earlier?

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 22, 2022

Thanks for the reminder, should get to it tomorrow

@rust-log-analyzer

This comment has been minimized.

@@ -23,3 +22,4 @@ cat /tmp/toolstate/toolstates.json
python3 "$X_PY" test --stage 2 check-tools
python3 "$X_PY" test --stage 2 src/tools/clippy
python3 "$X_PY" test --stage 2 src/tools/rustfmt
python3 "$X_PY" test --stage 2 src/tools/miri
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that this only gave us back test coverage on Linux, but not on Windows? That's a problem...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet