-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Bounds-check with PtrMetadata instead of Len in MIR #133734
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ fn main() { | |
let v1 = safe::as_mut_slice(&v); | ||
let v2 = safe::as_mut_slice(&v); | ||
v1[1] = 5; | ||
//~[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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to add a slice index expression in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it's a _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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, no, the read I added to the array case has I guess if I was to mark the Alternatively, AFAIK this is always dealing in a place where the projection chain starts with a deref, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm afraid I don't know much about MIR borrowck. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I think that's how the desugaring markers usually work, but I am not sure. |
||
v2[1] = 7; | ||
//~[tree]^ ERROR: /write access through .* is forbidden/ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// MIR for `index_array` after built | ||
|
||
fn index_array(_1: &[i32; 7], _2: usize) -> &i32 { | ||
debug array => _1; | ||
debug index => _2; | ||
let mut _0: &i32; | ||
let _3: &i32; | ||
let _4: usize; | ||
let mut _5: bool; | ||
|
||
bb0: { | ||
StorageLive(_3); | ||
StorageLive(_4); | ||
_4 = copy _2; | ||
FakeRead(ForIndex, (*_1)); | ||
_5 = Lt(copy _4, const 7_usize); | ||
assert(move _5, "index out of bounds: the length is {} but the index is {}", const 7_usize, copy _4) -> [success: bb1, unwind: bb2]; | ||
} | ||
|
||
bb1: { | ||
_3 = &(*_1)[_4]; | ||
_0 = &(*_3); | ||
StorageDead(_4); | ||
StorageDead(_3); | ||
return; | ||
} | ||
|
||
bb2 (cleanup): { | ||
resume; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// MIR for `index_const_generic_array` after built | ||
|
||
fn index_const_generic_array(_1: &[i32; N], _2: usize) -> &i32 { | ||
debug array => _1; | ||
debug index => _2; | ||
let mut _0: &i32; | ||
let _3: &i32; | ||
let _4: usize; | ||
let mut _5: bool; | ||
|
||
bb0: { | ||
StorageLive(_3); | ||
StorageLive(_4); | ||
_4 = copy _2; | ||
FakeRead(ForIndex, (*_1)); | ||
_5 = Lt(copy _4, const N); | ||
assert(move _5, "index out of bounds: the length is {} but the index is {}", const N, copy _4) -> [success: bb1, unwind: bb2]; | ||
} | ||
|
||
bb1: { | ||
_3 = &(*_1)[_4]; | ||
_0 = &(*_3); | ||
StorageDead(_4); | ||
StorageDead(_3); | ||
return; | ||
} | ||
|
||
bb2 (cleanup): { | ||
resume; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// MIR for `index_custom` after built | ||
|
||
fn index_custom(_1: &WithSliceTail, _2: usize) -> &i32 { | ||
debug custom => _1; | ||
debug index => _2; | ||
let mut _0: &i32; | ||
let _3: &i32; | ||
let _4: usize; | ||
let mut _5: *const [i32]; | ||
let mut _6: usize; | ||
let mut _7: bool; | ||
|
||
bb0: { | ||
StorageLive(_3); | ||
StorageLive(_4); | ||
_4 = copy _2; | ||
_5 = &raw const ((*_1).1: [i32]); | ||
_6 = PtrMetadata(move _5); | ||
_7 = Lt(copy _4, copy _6); | ||
assert(move _7, "index out of bounds: the length is {} but the index is {}", move _6, copy _4) -> [success: bb1, unwind: bb2]; | ||
} | ||
|
||
bb1: { | ||
_3 = &((*_1).1: [i32])[_4]; | ||
_0 = &(*_3); | ||
StorageDead(_4); | ||
StorageDead(_3); | ||
return; | ||
} | ||
|
||
bb2 (cleanup): { | ||
resume; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
// MIR for `index_mut_slice` after built | ||
|
||
fn index_mut_slice(_1: &mut [i32], _2: usize) -> &i32 { | ||
debug slice => _1; | ||
debug index => _2; | ||
let mut _0: &i32; | ||
let _3: &i32; | ||
let _4: usize; | ||
let mut _5: *const [i32]; | ||
let mut _6: usize; | ||
let mut _7: bool; | ||
|
||
bb0: { | ||
StorageLive(_3); | ||
StorageLive(_4); | ||
_4 = copy _2; | ||
_5 = &raw const (*_1); | ||
_6 = PtrMetadata(move _5); | ||
_7 = Lt(copy _4, copy _6); | ||
assert(move _7, "index out of bounds: the length is {} but the index is {}", move _6, copy _4) -> [success: bb1, unwind: bb2]; | ||
} | ||
|
||
bb1: { | ||
_3 = &(*_1)[_4]; | ||
_0 = &(*_3); | ||
StorageDead(_4); | ||
StorageDead(_3); | ||
return; | ||
} | ||
|
||
bb2 (cleanup): { | ||
resume; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, and there's no justification here? What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, there's a distinction between
ty::ConstKind::Unevaluated(ty::UnevaluatedConst(..))
andmir::ConstKind::Unevaluated(..)
and I think this may be getting that wrong. But generally randomly changing a function to do something different without asking why it's that way that seems like it may introduce subtle bugs 🤔