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

Miri sync #102573

Merged
merged 44 commits into from
Oct 6, 2022
Merged

Miri sync #102573

merged 44 commits into from
Oct 6, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 2, 2022

This is a Miri sync created with my experimental fork of josh. We should probably not merge this yet, but we can use this to check if the sync looks the way it should.

r? @oli-obk

RalfJung and others added 20 commits September 21, 2022 20:11
readme: tag-gc tweaks

r? `@saethlin`

Is this option truly needed often enough that it should be in the 'common' section? If not I vote for moving it to the 2nd section. Also `10,000` is a confusing way to write numbers in an international context (in many languages, `,` in a number plays the role of the point in English number notation, so using a space or underscore is less likely to lead to confusion).
GC: factor out visiting all machine values

`@saethlin` that is roughly what I had in mind.

I think some parts of the state are skipped by the visitor. I listed the ones that I found in FIXMEs but I am not sure if that list is complete.
run all extern-so tests consistently without dependencies
Don't back up past the caller when looking for an FnEntry span

Fixes rust-lang/miri#2536

This adds a fix for the logic as well as a regression test. In the new test `tests/fail/stacked_borrows/fnentry_invalidation2.rs`, before this PR, we display this diagnostic:
```
help: <3278> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:13:5
   |
13 |     inner(&mut t);
   |     ^^^^^^^^^^^^^
```
Which is very misleading. It is not this call itself, but what happens within the call that invalidates the tag we want. With this PR, we get:
```
help: <2798> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag inside this call
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:20:13
   |
20 |     let _ = t.sli.as_mut_ptr();
   |             ^^^^^^^^^^^^^^^^^^
```
Which is much better.
remove macOS work-around that is no longer needed

Judging from actions/cache#403 it sounds like this work-around is not needed any more.
CI: use cargo sparse registry

CI spends a few minutes downloading the index so this could help.
Add flag to specify the number of cpus

Apparently you can't rename a branch from github's website without it closing all your PRs with that branch. So this is  just rust-lang#2545
@rustbot
Copy link
Collaborator

rustbot commented Oct 2, 2022

The Miri subtree was changed

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 2, 2022
@RalfJung RalfJung mentioned this pull request Oct 2, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Oct 2, 2022

Also this does not merge the entire recent Miri history, but only the parts before rust-lang/miri#2554 -- that PR will probably have to be edited out of the history via a force-push, and the rest then re-constructed by hand.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

I'll land rust-lang/miri#2566 once CI is green, so it's worth waiting for that IMO. I'll also use that to test re-syncing a bit.

Expand VisitMachineValues to cover more pointers in the interpreter

Follow-on to rust-lang/miri#2559

This is making me want to write a proc macro 🤔

r? `@RalfJung`
@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

Okay, syncing onto an already existing sync branch is not a thing -- josh will complain

remote: hint: Updates were rejected because a pushed branch tip is behind its remote
remote: hint: counterpart. Check out this branch and integrate the remote changes
remote: hint: (e.g. 'git pull ...') before pushing again.
remote: hint: See the 'Note about fast-forwards' in 'git push --help' for details.

I don't quite understand why this is a problem, given that even when it succeeds the pushed tip does not contain a sync from the rustc repo, but 🤷 . We want to always use the 'remote counterpart does not exist yet, use -o base' mode, I think.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

There is something I don't quite understand though... josh decided to base this branch off of 8e3b9bc, which is quite a bit behind the current master. I did update my master branch to the latest one first so it could have picked a newer one. There seems to be nothing wrong with using that particular commit, but I'd like to understand why it didn't use the latest one...

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

On the plus side, if I merge this with master, and then re-extract Miri, I do get something that is compatible with rust-lang/miri#2583. I get a commit with 2 parents: current miri master (the one I used as basis for this PR), and 338c50e which is the commit josh produced for the previous sync. So, it looks perfect.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between ead49f0beb7e36007aeed59f862f10f72b889c59 and cc84daaebc6456d8109cddd3b837fa4d138e679c
Tool subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
tests/pass/available-parallelism.rs ... ok
tests/pass/associated-const.rs ... ok
tests/pass/bools.rs ... ok
tests/pass/assume_bug.rs ... ok
tests/pass/available-parallelism-miri-num-cpus.rs ... ok
tests/pass/align.rs ... ok
tests/pass/atomic-compare-exchange-weak-never-fail.rs ... ok
tests/pass/async-fn.rs ... ok
tests/pass/arrays.rs ... ok
---
.......... (70/77)
.......    (77/77)


/checkout/src/test/rustdoc-gui/basic-code.goml basic-code... FAILED
[ERROR] (line 3) expected 1 elements, found 0: for command `assert-count: (".src-line-numbers", 1)`
Build completed unsuccessfully in 0:02:17

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

/checkout/src/test/rustdoc-gui/basic-code.goml basic-code... FAILED

looks unrelated

@oli-obk
Copy link
Contributor

oli-obk commented Oct 4, 2022

The new diff lgtm. r=me unless you were still waiting on sth

@RalfJung
Copy link
Member Author

RalfJung commented Oct 4, 2022

The base commit is a bit strange, but it doesn't affect the extracted history... so it's not relevant for roundtrips. (Future versions of josh making different choices here would not be a problem for us.) If we don't mind the rustc history graph becoming even more non-tree-like (the commit this is based on is not a bors-created merge commit, it's a commit from inside a PR), then I think we can land this.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2022

After reproducing the exact same commit with the alternative implementation of the required josh features from josh-project/josh#965, I am confident that this all holds together.

@bors r=oli-obk

@RalfJung
Copy link
Member Author

RalfJung commented Oct 5, 2022

@bors ping
hello, bors?

@bors
Copy link
Contributor

bors commented Oct 5, 2022

📌 Commit 9cc11e2 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 Oct 5, 2022
@bors
Copy link
Contributor

bors commented Oct 5, 2022

😪 I'm awake I'm awake

@bors
Copy link
Contributor

bors commented Oct 6, 2022

⌛ Testing commit 9cc11e2 with merge 27579a2...

@bors
Copy link
Contributor

bors commented Oct 6, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2022
@bors bors merged commit 27579a2 into rust-lang:master Oct 6, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 6, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (27579a2): 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
Regressions ❌
(secondary)
2.2% [1.4%, 3.5%] 8
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.6% [-6.0%, -2.9%] 3
All ❌✅ (primary) - - 0

Cycles

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

Footnotes

  1. the arithmetic mean of the percent change

  2. number of relevant changes

@RalfJung
Copy link
Member Author

RalfJung commented Oct 6, 2022

Good riddance, xargo, and thanks for all the fish. :)

@RalfJung RalfJung deleted the mirisync branch October 6, 2022 07:15
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Miri sync

This is a Miri sync created with my experimental fork of josh. We should probably not merge this yet, but we can use this to check if the sync looks the way it should.

r? `@oli-obk`
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
Development

Successfully merging this pull request may close these issues.

9 participants