-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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 all commits
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 |
---|---|---|
|
@@ -1163,6 +1163,9 @@ pub enum DesugaringKind { | |
WhileLoop, | ||
/// `async Fn()` bound modifier | ||
BoundModifier, | ||
/// Marks a `&raw const *_1` needed as part of getting the length of a mutable | ||
/// slice for the bounds check, so that MIRI's retag handling can recognize it. | ||
IndexBoundsCheckReborrow, | ||
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. Span should definitely not be used like this. Span desugarings should only ever be used for diagnostics and things like that -- making miri rely on this behavior seems really bad. If we need to track certain locals for the purposes of some special miri hack, this needs to be recorded in the MIR body separately. 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. It's a pretty bad hack, yeah. Medium-term I hope that Miri can entirely stop doing retagging on |
||
} | ||
|
||
impl DesugaringKind { | ||
|
@@ -1179,6 +1182,7 @@ impl DesugaringKind { | |
DesugaringKind::ForLoop => "`for` loop", | ||
DesugaringKind::WhileLoop => "`while` loop", | ||
DesugaringKind::BoundModifier => "trait bound modifier", | ||
DesugaringKind::IndexBoundsCheckReborrow => "slice indexing", | ||
} | ||
} | ||
} | ||
|
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; | ||
} | ||
} |
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 🤔