Bounds-check with PtrMetadata instead of Len in MIR #133734
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE machinery cc @rust-lang/wg-const-eval |
| - _7 = Len((*_2)); | ||
| - _8 = Lt(copy _6, copy _7); | ||
| - assert(move _8, "index out of bounds: the length is {} but the index is {}", move _7, copy _6) -> [success: bb1, unwind unreachable]; | ||
| + _7 = const 3_usize; | ||
| + _8 = const true; | ||
| + assert(const true, "index out of bounds: the length is {} but the index is {}", const 3_usize, const 1_usize) -> [success: bb1, unwind unreachable]; |
There was a problem hiding this comment.
I ended up just deleting this test, which seemed fine to me because GVN can do similar things to this, and DataflowConstProp is off in release anyway.
| _11 = Len((*_1)); | ||
| _12 = Lt(copy _10, copy _11); | ||
| assert(move _12, "index out of bounds: the length is {} but the index is {}", move _11, copy _10) -> [success: bb6, unwind unreachable]; | ||
| _11 = Lt(copy _10, copy _3); | ||
| assert(move _11, "index out of bounds: the length is {} but the index is {}", copy _3, copy _10) -> [success: bb6, unwind unreachable]; |
There was a problem hiding this comment.
And overall the MIR seems slightly better after this PR, since [T]::len had already moved to PtrMetadata, and thus using it here too means that GVN can dedup the PtrMetadata from the bounds check with the one from the .len() call in the source.
This comment has been minimized.
This comment has been minimized.
|
Looks like I need to rethink this, because it seems borrowck and initialization checking are somehow coupled to this lowering. EDIT: actually, maybe I just need a fake read. |
scottmcm
force-pushed
the
lower-indexing-to-ptrmetadata
branch
2 times, most recently
from
2471745
to
a7f9bf7
Compare
This comment has been minimized.
This comment has been minimized.
scottmcm
force-pushed
the
lower-indexing-to-ptrmetadata
branch
from
a7f9bf7
to
90b3895
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
scottmcm
force-pushed
the
lower-indexing-to-ptrmetadata
branch
from
90b3895
to
612adbb
Compare
| //~[stack]^ ERROR: /write access .* tag does not exist in the borrow stack/ | ||
| //~[stack]^ ERROR: /trying to retag .+ for SharedReadOnly permission .+ tag does not exist in the borrow stack for this location/ |
There was a problem hiding this comment.
@RalfJung I think what's happened here is that SB is now failing "on" the read of the slice length for the bounds check, before it gets to trying to write, thus it mentioning SharedReadOnly instead of "write access", which seems at least plausibly fine. But please double-check that I didn't blow this up in some horrible way 😬
There was a problem hiding this comment.
Hm, yeah as you say it seems to fail on a read / retag-for-read now, instead of a write. It is a bit surprising that this would change now. How did the MIR change here?
There was a problem hiding this comment.
Would be good to add a slice index expression in mir-opt/retag.rs so that we can see how this looks like with retagging.
There was a problem hiding this comment.
Because it's a &mut [_], it changed from doing the bounds check as
_2 = Len(*_1);
_3 = Lt(const 1_usize, _2)
assert(_3, …);to
_4 = &raw const *_1;
_2 = PtrMetadata(move _4);
_3 = Lt(const 1_usize, _2)
assert(_3, …);(It's not just PtrMetadata(copy _1) because there could be a projection [like Len((*_1).3)] and because that copy on a &mut ends up breaking borrowck.)
There was a problem hiding this comment.
Ah, yeah the &raw const here is relevant for Stacked Borrows -- it basically acts as a read on the entire slice. That is a bit unfortunate since so far, for slice accesses, we didn't do such a whole-slice retag. I think ideally we would not add a retag to these raw reborrows.
Is there any way for the retag pass to recognize these reborrows? Like, are they marked as syntactic sugar somewhere, or so? I recall we do that for some constructs, but I am not actually sure how it is represented.
Long-term I hope we can change SB to make raw retags true NOPs, but that will require a bit more work.
Rather than emitting
Len(*_n)in array index bounds checks, emitPtrMetadata(copy _n)instead -- with some asterisks for arrays and&mutthat need it to be done slightly differently.We're getting pretty close to removing
Lenentirely, actually. I think just one more PR after this (for slice drop shims).r? mir