-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Get rid of the Scalar
and ScalarPair
variants of ConstValue
...
#55260
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 2 commits
edf94c5
856eaed
fe4e950
04fc561
b11b3cb
3aecb0d
8d42376
847f5b9
e52644c
8246dd4
5620d68
fc6472c
336094f
b4ba332
dfc5d26
c50d77d
1d14a84
9fbce39
1f2fcee
e3bbe6d
6ff70da
7b526d8
9df63e9
a7a92e2
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 |
---|---|---|
|
@@ -28,7 +28,7 @@ pub use self::value::{Scalar, ConstValue}; | |
use std::fmt; | ||
use mir; | ||
use hir::def_id::DefId; | ||
use ty::{self, TyCtxt, Instance}; | ||
use ty::{self, TyCtxt, Instance, Ty, ParamEnvAnd}; | ||
use ty::layout::{self, Align, HasDataLayout, Size}; | ||
use middle::region; | ||
use std::iter; | ||
|
@@ -545,7 +545,7 @@ pub struct Allocation<Tag=(),Extra=()> { | |
pub extra: Extra, | ||
} | ||
|
||
impl<Tag, Extra: Default> Allocation<Tag, Extra> { | ||
impl<Tag: Copy, Extra: Default> Allocation<Tag, Extra> { | ||
/// Creates a read-only allocation initialized by the given bytes | ||
pub fn from_bytes(slice: &[u8], align: Align) -> Self { | ||
let mut undef_mask = UndefMask::new(Size::ZERO); | ||
|
@@ -575,6 +575,144 @@ impl<Tag, Extra: Default> Allocation<Tag, Extra> { | |
extra: Extra::default(), | ||
} | ||
} | ||
|
||
#[inline] | ||
pub fn size(&self) -> Size { | ||
Size::from_bytes(self.bytes.len() as u64) | ||
} | ||
|
||
pub fn check_align( | ||
&self, | ||
offset: Size, | ||
required_align: Align, | ||
) -> EvalResult<'tcx> { | ||
if self.align.abi() > required_align.abi() { | ||
RalfJung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return err!(AlignmentCheckFailed { | ||
has: self.align, | ||
required: required_align, | ||
}); | ||
} | ||
let offset = offset.bytes(); | ||
if offset % required_align.abi() == 0 { | ||
Ok(()) | ||
} else { | ||
let has = offset % required_align.abi(); | ||
err!(AlignmentCheckFailed { | ||
has: Align::from_bytes(has, has).unwrap(), | ||
required: required_align, | ||
}) | ||
} | ||
} | ||
|
||
pub fn check_bounds( | ||
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. This needs a doc comment defining "inbounds". Is one-past-the-end accepted or not? |
||
&self, | ||
offset: Size, | ||
size: Size, | ||
access: bool, | ||
) -> EvalResult<'tcx> { | ||
let end = offset + size; | ||
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. What if this overflows? (Same below in |
||
let allocation_size = self.size(); | ||
if end > allocation_size { | ||
err!(PointerOutOfBounds { offset, access, allocation_size }) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
|
||
pub fn check_defined( | ||
&self, | ||
offset: Size, | ||
size: Size, | ||
) -> EvalResult<'tcx> { | ||
self.undef_mask.is_range_defined( | ||
offset, | ||
offset + size, | ||
).or_else(|idx| err!(ReadUndefBytes(idx))) | ||
} | ||
|
||
pub fn check_relocations( | ||
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. What exactly is this "checking"? I spent quite some time giving all of these methods extensive docs in |
||
&self, | ||
hdl: impl HasDataLayout, | ||
offset: Size, | ||
size: Size, | ||
) -> EvalResult<'tcx> { | ||
if self.relocations(hdl, offset, size)?.len() != 0 { | ||
err!(ReadPointerAsBytes) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
|
||
pub fn relocations( | ||
&self, | ||
hdl: impl HasDataLayout, | ||
offset: Size, | ||
size: Size, | ||
) -> EvalResult<'tcx, &[(Size, (Tag, AllocId))]> { | ||
// We have to go back `pointer_size - 1` bytes, as that one would still overlap with | ||
// the beginning of this range. | ||
let start = offset.bytes().saturating_sub(hdl.pointer_size().bytes() - 1); | ||
let end = offset + size; // this does overflow checking | ||
Ok(self.relocations.range(Size::from_bytes(start)..end)) | ||
} | ||
|
||
pub fn get_bytes( | ||
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. Needs a doc comment explaining exactly which checks are performed, and which not. (Internal relocations? Relocations on the edges?) Also, if |
||
&self, | ||
hdl: impl HasDataLayout, | ||
offset: Size, | ||
size: Size, | ||
required_align: Align, | ||
) -> EvalResult<'tcx, &[u8]> { | ||
self.check_align(offset, required_align)?; | ||
self.check_bounds(offset, size, true)?; | ||
self.check_defined(offset, size)?; | ||
self.check_relocations(hdl, offset, size)?; | ||
Ok(self.bytes_ignoring_relocations_and_undef(offset, size)) | ||
} | ||
|
||
pub fn read_bits( | ||
&self, | ||
tcx: TyCtxt<'_, '_, 'tcx>, | ||
offset: Size, | ||
ty: ParamEnvAnd<'tcx, Ty<'tcx>>, | ||
) -> EvalResult<'tcx, u128> { | ||
let ty = tcx.lift_to_global(&ty).unwrap(); | ||
let layout = tcx.layout_of(ty).unwrap_or_else(|e| { | ||
panic!("could not compute layout for {:?}: {:?}", ty, e) | ||
}); | ||
let bytes = self.get_bytes(tcx, offset, layout.size, layout.align)?; | ||
Ok(read_target_uint(tcx.data_layout.endian, bytes).unwrap()) | ||
} | ||
|
||
pub fn read_scalar( | ||
&self, | ||
hdl: impl HasDataLayout, | ||
offset: Size, | ||
) -> EvalResult<'tcx, Scalar<Tag>> { | ||
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. Wait, why does (See, a doc comment would have been useful. ;) 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. this function is called 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 did the diff view fool me? Yes it did. I am still quite confused why this looks so different from what we do in memory.rs? |
||
let size = hdl.data_layout().pointer_size; | ||
let required_align = hdl.data_layout().pointer_align; | ||
self.check_align(offset, required_align)?; | ||
self.check_bounds(offset, size, true)?; | ||
self.check_defined(offset, size)?; | ||
let bytes = self.bytes_ignoring_relocations_and_undef(offset, size); | ||
let offset = read_target_uint(hdl.data_layout().endian, &bytes).unwrap(); | ||
let offset = Size::from_bytes(offset as u64); | ||
if let Some(&(tag, alloc_id)) = self.relocations.get(&offset) { | ||
Ok(Pointer::new_with_tag(alloc_id, offset, tag).into()) | ||
} else { | ||
Ok(Scalar::Bits { | ||
bits: offset.bytes() as u128, | ||
size: size.bytes() as u8, | ||
}) | ||
} | ||
} | ||
|
||
fn bytes_ignoring_relocations_and_undef(&self, offset: Size, size: Size) -> &[u8] { | ||
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. And also ignoring alignment and bounds... wtf? We didn't have anything nearly as unsafe before. |
||
let end = offset + size; | ||
let offset = offset.bytes() as usize; | ||
let end = end.bytes() as usize; | ||
&self.bytes[offset..end] | ||
} | ||
} | ||
|
||
impl<'tcx> ::serialize::UseSpecializedDecodable for &'tcx Allocation {} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,8 +10,9 @@ | |
|
||
#![allow(unknown_lints)] | ||
|
||
use ty::layout::{HasDataLayout, Size}; | ||
use ty::layout::{HasDataLayout, Align, Size, TyLayout}; | ||
use ty::subst::Substs; | ||
use ty; | ||
use hir::def_id::DefId; | ||
|
||
use super::{EvalResult, Pointer, PointerArithmetic, Allocation, AllocId, sign_extend, truncate}; | ||
|
@@ -25,57 +26,130 @@ pub enum ConstValue<'tcx> { | |
/// evaluation | ||
Unevaluated(DefId, &'tcx Substs<'tcx>), | ||
|
||
/// Used only for types with layout::abi::Scalar ABI and ZSTs | ||
/// | ||
/// Not using the enum `Value` to encode that this must not be `Undef` | ||
Scalar(Scalar), | ||
|
||
/// Used only for *fat pointers* with layout::abi::ScalarPair | ||
/// | ||
/// Needed for pattern matching code related to slices and strings. | ||
ScalarPair(Scalar, Scalar), | ||
|
||
/// An allocation + offset into the allocation. | ||
/// Invariant: The AllocId matches the allocation. | ||
ByRef(AllocId, &'tcx Allocation, Size), | ||
} | ||
|
||
impl<'tcx> ConstValue<'tcx> { | ||
#[inline] | ||
pub fn try_to_scalar(&self) -> Option<Scalar> { | ||
match *self { | ||
ConstValue::Unevaluated(..) | | ||
ConstValue::ByRef(..) | | ||
ConstValue::ScalarPair(..) => None, | ||
ConstValue::Scalar(val) => Some(val), | ||
pub fn try_as_by_ref(&self) -> Option<(AllocId, &'tcx Allocation, Size)> { | ||
match self { | ||
ConstValue::Unevaluated(..) => None, | ||
ConstValue::ByRef(a, b, c) => Some((*a, *b, *c)), | ||
} | ||
} | ||
|
||
#[inline] | ||
pub fn try_to_bits(&self, size: Size) -> Option<u128> { | ||
self.try_to_scalar()?.to_bits(size).ok() | ||
/// if this is ByRef, return the same thing but with the offset increased by `n` | ||
pub fn try_offset(&self, n: Size) -> Option<Self> { | ||
let (id, alloc, offset) = self.try_as_by_ref()?; | ||
Some(ConstValue::ByRef(id, alloc, offset + n)) | ||
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. We have complex and subtle code to do overflow-aware (which is architecture-dependent) pointer arithmetic. We surely shouldn't just do plain addition here. |
||
} | ||
|
||
#[inline] | ||
pub fn try_get_bytes(&self, hdl: impl HasDataLayout, n: Size, align: Align) -> Option<&[u8]> { | ||
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.
|
||
let (_, alloc, offset) = self.try_as_by_ref()?; | ||
alloc.get_bytes(hdl, offset, n, align).ok() | ||
} | ||
|
||
#[inline] | ||
pub fn try_to_bits(&self, hdl: impl HasDataLayout, layout: TyLayout<'tcx>) -> Option<u128> { | ||
let bytes = self.try_get_bytes(hdl, layout.size, layout.align)?; | ||
let endian = hdl.data_layout().endian; | ||
super::read_target_uint(endian, &bytes).ok() | ||
} | ||
|
||
#[inline] | ||
pub fn try_to_ptr(&self) -> Option<Pointer> { | ||
self.try_to_scalar()?.to_ptr().ok() | ||
pub fn try_to_usize(&self, hdl: impl HasDataLayout) -> Option<u128> { | ||
let size = hdl.data_layout().pointer_size; | ||
let align = hdl.data_layout().pointer_align; | ||
let bytes = self.try_get_bytes(hdl, size, align)?; | ||
let endian = hdl.data_layout().endian; | ||
super::read_target_uint(endian, &bytes).ok() | ||
} | ||
|
||
#[inline] | ||
pub fn try_to_ptr( | ||
&self, | ||
hdl: impl HasDataLayout, | ||
) -> Option<Pointer> { | ||
let (_, alloc, offset) = self.try_as_by_ref()?; | ||
alloc.read_scalar(hdl, offset).ok()?.to_ptr().ok() | ||
} | ||
|
||
/// e.g. for vtables, fat pointers or single pointers | ||
#[inline] | ||
pub fn new_pointer_list( | ||
list: &[Scalar], | ||
tcx: ty::TyCtxt<'_, '_, 'tcx>, | ||
) -> Self { | ||
let ps = tcx.data_layout().pointer_size; | ||
let mut alloc = Allocation::undef( | ||
ps * list.len() as u64, | ||
tcx.data_layout().pointer_align, | ||
); | ||
alloc.undef_mask.set_range_inbounds(Size::ZERO, ps * list.len() as u64, true); | ||
for (i, s) in list.iter().enumerate() { | ||
let (int, ptr) = match s { | ||
Scalar::Bits { bits, size } => { | ||
assert!(*size as u64 == ps.bytes()); | ||
(*bits as u64, None) | ||
} | ||
Scalar::Ptr(ptr) => (ptr.offset.bytes(), Some(ptr)), | ||
}; | ||
let i = i * ps.bytes() as usize; | ||
let j = i + ps.bytes() as usize; | ||
super::write_target_uint( | ||
tcx.data_layout().endian, | ||
&mut alloc.bytes[i..j], | ||
int.into(), | ||
).unwrap(); | ||
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. Uh, so here we are duplicating |
||
if let Some(ptr) = ptr { | ||
alloc.relocations.insert( | ||
ps * i as u64, | ||
(ptr.tag, ptr.alloc_id), | ||
); | ||
} | ||
} | ||
Self::from_allocation(tcx, alloc) | ||
} | ||
|
||
#[inline] | ||
pub fn from_allocation( | ||
tcx: ty::TyCtxt<'_, '_, 'tcx>, | ||
alloc: Allocation, | ||
) -> Self { | ||
let alloc = tcx.intern_const_alloc(alloc); | ||
let alloc_id = tcx.alloc_map.lock().allocate(alloc); | ||
ConstValue::ByRef(alloc_id, alloc, Size::ZERO) | ||
} | ||
|
||
#[inline] | ||
pub fn new_slice( | ||
val: Scalar, | ||
len: u64, | ||
cx: impl HasDataLayout | ||
tcx: ty::TyCtxt<'_, '_, 'tcx>, | ||
) -> Self { | ||
ConstValue::ScalarPair(val, Scalar::Bits { | ||
bits: len as u128, | ||
size: cx.data_layout().pointer_size.bytes() as u8, | ||
}) | ||
Self::new_pointer_list( | ||
&[ | ||
val, | ||
Scalar::Bits { | ||
bits: len as u128, | ||
size: tcx.data_layout.pointer_size.bytes() as u8, | ||
}, | ||
], | ||
tcx, | ||
) | ||
} | ||
|
||
#[inline] | ||
pub fn new_dyn_trait(val: Scalar, vtable: Pointer) -> Self { | ||
ConstValue::ScalarPair(val, Scalar::Ptr(vtable)) | ||
pub fn new_dyn_trait( | ||
val: Scalar, | ||
vtable: Pointer, | ||
tcx: ty::TyCtxt<'_, '_, 'tcx>, | ||
) -> Self { | ||
Self::new_pointer_list(&[val, vtable.into()], tcx) | ||
} | ||
} | ||
|
||
|
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.
But this way we lose the information which allocation it pointed to, right?
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.
yes
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.
Why did you do that?
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.
Allocation
s don't know whichAllocId
s (there might be multiple) point to it. So if I have a method on anAllocation
, I can't create an error containing anAllocId
except by passing it as an argument just used for the errorThere 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.
That's not a great regression, but well. :/
However:
Eh, what?!??
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.
we can store the AllocId of an Allocation in the Allocation itself if you prefer that
that can happen easily as we don't have a deduplication cache anymore. We used to have one, but it got removed during some parallel rustc PRs
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.
Deduplication of what? We are still interning, which deduplicates, right?
I think for now I can live with this regression, it's not like the
AllocId
would mean terribly much to most users. It was sometimes helpful during debugging, but only with a full trace and that trace would probably still have the ID somewhere right before the error.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.
Creating
AllocId
s for string literals will deduplicate the memory of the content, but still generate a newAllocId