Previous Stacked Borrows diagnostics were missing a lot of information
about the state of the interpreter, and it was difficult to add
additional state because it was threaded through all the intervening
function signatures.
This change factors a lot of the arguments which used to be passed
individually to many stacked borrows functions into a single
`DiagnosticCx`, which is built in `Stacks::for_each`, and since it
wraps a handle to `AllocHistory`, we can now handle more nuanced
things like heterogeneous borrow of `!Freeze` types.
reborrow error: clarify that we are reborrowing *from* that tag
`@saethlin` I found the current message not entirely clear, so what do you think about this?
Optimizing Stacked Borrows (part 2): Shrink Item
This moves protectors out of `Item`, storing them both in a global `HashSet` which contains all currently-protected tags as well as a `Vec<SbTag>` on each `Frame` so that when we return from a function we know which tags to remove from the protected set.
This also bit-packs the 64-bit tag and the 2-bit permission together when they are stored in memory. This means we theoretically run out of tags sooner, but I doubt that limit will ever be hit.
Together these optimizations reduce the memory footprint of Miri when executing programs which stress Stacked Borrows by ~66%. For example, running a test with isolation off which only panics currently peaks at ~19 GB, with this PR it peaks at ~6.2 GB.
To-do
- [x] Enforce the 62-bit limit
- [x] Decide if there is a better order to pack the tag and permission in
- [x] Wait for `UnsafeCell` to become infectious, or express offsets + tags in the global protector set
Benchmarks before:
```
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/backtraces/Cargo.toml
Time (mean ± σ): 8.948 s ± 0.253 s [User: 8.752 s, System: 0.158 s]
Range (min … max): 8.619 s … 9.279 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/mse/Cargo.toml
Time (mean ± σ): 2.129 s ± 0.037 s [User: 1.849 s, System: 0.248 s]
Range (min … max): 2.086 s … 2.176 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/serde1/Cargo.toml
Time (mean ± σ): 3.334 s ± 0.017 s [User: 3.211 s, System: 0.103 s]
Range (min … max): 3.315 s … 3.352 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/serde2/Cargo.toml
Time (mean ± σ): 3.316 s ± 0.038 s [User: 3.207 s, System: 0.095 s]
Range (min … max): 3.282 s … 3.375 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/unicode/Cargo.toml
Time (mean ± σ): 6.391 s ± 0.323 s [User: 5.928 s, System: 0.412 s]
Range (min … max): 6.090 s … 6.917 s 5 runs
```
After:
```
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/backtraces/Cargo.toml
Time (mean ± σ): 6.955 s ± 0.051 s [User: 6.807 s, System: 0.132 s]
Range (min … max): 6.900 s … 7.038 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/mse/Cargo.toml
Time (mean ± σ): 1.784 s ± 0.012 s [User: 1.627 s, System: 0.156 s]
Range (min … max): 1.772 s … 1.797 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/serde1/Cargo.toml
Time (mean ± σ): 2.505 s ± 0.095 s [User: 2.311 s, System: 0.096 s]
Range (min … max): 2.405 s … 2.603 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/serde2/Cargo.toml
Time (mean ± σ): 2.449 s ± 0.031 s [User: 2.306 s, System: 0.100 s]
Range (min … max): 2.395 s … 2.467 s 5 runs
Benchmark 1: cargo +miri miri run --manifest-path bench-cargo-miri/unicode/Cargo.toml
Time (mean ± σ): 3.667 s ± 0.110 s [User: 3.498 s, System: 0.140 s]
Range (min … max): 3.564 s … 3.814 s 5 runs
```
The decrease in system time is probably due to spending less time in the page fault handler.
stacked_borrow now has an item module, and its own FrameExtra. These
serve to protect the implementation of Item (which is a bunch of
bit-packing tricks) from the primary logic of Stacked Borrows, and the
FrameExtra we have separates Stacked Borrows more cleanly from the
interpreter itself.
The new strategy for checking protectors also makes some subtle
performance tradeoffs, so they are now documented in Stack::item_popped
because that function primarily benefits from them, and it also touches
every aspect of them.
Also separating the actual CallId that is protecting a Tag from the Tag
makes it inconvienent to reproduce exactly the same protector errors, so
this also takes the opportunity to use some slightly cleaner English in
those errors. We need to make some change, might as well make it good.
Previously, Item was a struct of a NonZeroU64, an Option which was
usually unset or irrelevant, and a 4-variant enum. So collectively, the
size of an Item was 24 bytes, but only 8 bytes were used for the most
part.
So this takes advantage of the fact that it is probably impossible to
exhaust the total space of SbTags, and steals 3 bits from it to pack the
whole struct into a single u64. This bit-packing means that we reduce
peak memory usage when Miri goes memory-bound by ~3x. We also get CPU
performance improvements of varying size, because not only are we simply
accessing less memory, we can now compare a Vec<Item> using a memcmp
because it does not have any padding.
Format tests with rustfmt (225-275 of 300)
Extracted from #2097.
These cases all involve a line comment at the end of a block that rustfmt has chosen to wrap.
```diff
- unsafe { (*ptr).set(20); } //~ ERROR does not exist in the borrow stack
+ unsafe {
+ (*ptr).set(20);
+ } //~ ERROR does not exist in the borrow stack
```
I have moved all of those comments back onto the same line as the content of the block instead, as was indicated being `@RalfJung's` preference in https://github.com/rust-lang/miri/pull/2097#discussion_r862436672.
```diff
+ unsafe {
+ (*ptr).set(20); //~ ERROR does not exist in the borrow stack
+ }
```
Five of the files being skipped here are because rustfmt is buggy (see
the error messages below). The others have clearly preferable manual
formatting.
error[internal]: left behind trailing whitespace
--> tests/fail/validity/transmute_through_ptr.rs:18:18:1
|
18 |
| ^^^^
|
warning: rustfmt has failed to format. See previous 1 errors.
error[internal]: left behind trailing whitespace
--> tests/fail/stacked_borrows/illegal_read2.rs:10:10:1
|
10 |
| ^^^^
|
warning: rustfmt has failed to format. See previous 1 errors.
error[internal]: left behind trailing whitespace
--> tests/fail/stacked_borrows/illegal_read5.rs:15:15:1
|
15 |
| ^^^^
|
warning: rustfmt has failed to format. See previous 1 errors.
error[internal]: left behind trailing whitespace
--> tests/fail/stacked_borrows/illegal_read1.rs:10:10:1
|
10 |
| ^^^^
|
warning: rustfmt has failed to format. See previous 1 errors.
error[internal]: left behind trailing whitespace
--> tests/fail/erroneous_const2.rs:9:9:1
|
9 |
| ^^^^
|
warning: rustfmt has failed to format. See previous 1 errors.
