Skip to content
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

Get rid of the Scalar and ScalarPair variants of ConstValue... #55260

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
edf94c5
Update function names in comments
oli-obk Oct 19, 2018
856eaed
Simplify `ConstValue`
oli-obk Oct 21, 2018
fe4e950
Make the HashStable impl of Allocation safer for future changes of th…
oli-obk Oct 21, 2018
04fc561
The `Hash` and `Eq` impls of `ConstValue` were inherently broken
oli-obk Oct 21, 2018
b11b3cb
Fixup a bunch of things for "simplify const value"
oli-obk Oct 21, 2018
3aecb0d
Fix some simd intrinsics
oli-obk Oct 21, 2018
8d42376
Fix an ICE that can occur while errors are already being emitted
oli-obk Oct 21, 2018
847f5b9
Const prop is only interested in immediate constants right now
oli-obk Oct 21, 2018
e52644c
Update a few tests with their improved diagnostic messages
oli-obk Oct 21, 2018
8246dd4
Alignment check facepalm
oli-obk Oct 21, 2018
5620d68
Enums can produce scalar pairs with undefs in one of the elements
oli-obk Oct 21, 2018
fc6472c
Hide a constructor function that is unused outside this module
oli-obk Oct 22, 2018
336094f
Use a more appropriate parameter environment
oli-obk Oct 22, 2018
b4ba332
Don't resolve `impl Trait` in function def constants to the concrete …
oli-obk Oct 22, 2018
dfc5d26
Constants carry their type with them
oli-obk Oct 22, 2018
c50d77d
Alignment of `const_field` results must be of the field's type
oli-obk Oct 22, 2018
1d14a84
Add a debugging aid
oli-obk Oct 22, 2018
9fbce39
Change some useless `ParamEnv::empty` to `reveal_all`
oli-obk Oct 22, 2018
1f2fcee
Improve debugging output of layout computation failure
oli-obk Oct 22, 2018
e3bbe6d
Shrink allocations to fit the type at hand
oli-obk Oct 22, 2018
6ff70da
Resize `Allocation`s when doing field accesses into them
oli-obk Oct 22, 2018
7b526d8
Read the slice length from the correct place in the fat pointer
oli-obk Oct 22, 2018
9df63e9
Global Allocations don't have an `extra` value
oli-obk Oct 22, 2018
a7a92e2
Rebase fallout
oli-obk Oct 22, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions src/librustc/ich/impls_ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -375,13 +375,6 @@ for ::mir::interpret::ConstValue<'gcx> {
def_id.hash_stable(hcx, hasher);
substs.hash_stable(hcx, hasher);
}
Scalar(val) => {
val.hash_stable(hcx, hasher);
}
ScalarPair(a, b) => {
a.hash_stable(hcx, hasher);
b.hash_stable(hcx, hasher);
}
ByRef(id, alloc, offset) => {
id.hash_stable(hcx, hasher);
alloc.hash_stable(hcx, hasher);
Expand Down Expand Up @@ -463,13 +456,15 @@ impl<'a> HashStable<StableHashingContext<'a>> for mir::interpret::Allocation {
hcx: &mut StableHashingContext<'a>,
hasher: &mut StableHasher<W>,
) {
self.bytes.hash_stable(hcx, hasher);
for reloc in self.relocations.iter() {
trace!("hashing allocation {:?}", self);
let mir::interpret::Allocation { bytes, relocations, undef_mask, align, mutability } = self;
bytes.hash_stable(hcx, hasher);
for reloc in relocations.iter() {
reloc.hash_stable(hcx, hasher);
}
self.undef_mask.hash_stable(hcx, hasher);
self.align.hash_stable(hcx, hasher);
self.mutability.hash_stable(hcx, hasher);
undef_mask.hash_stable(hcx, hasher);
align.hash_stable(hcx, hasher);
mutability.hash_stable(hcx, hasher);
}
}

Expand Down Expand Up @@ -512,7 +507,7 @@ for ::mir::interpret::EvalErrorKind<'gcx, O> {
hasher: &mut StableHasher<W>) {
use mir::interpret::EvalErrorKind::*;

mem::discriminant(&self).hash_stable(hcx, hasher);
mem::discriminant(self).hash_stable(hcx, hasher);

match *self {
FunctionArgCountMismatch |
Expand Down Expand Up @@ -577,11 +572,11 @@ for ::mir::interpret::EvalErrorKind<'gcx, O> {
NoMirFor(ref s) => s.hash_stable(hcx, hasher),
UnterminatedCString(ptr) => ptr.hash_stable(hcx, hasher),
PointerOutOfBounds {
ptr,
offset,
access,
allocation_size,
} => {
ptr.hash_stable(hcx, hasher);
offset.hash_stable(hcx, hasher);
access.hash_stable(hcx, hasher);
allocation_size.hash_stable(hcx, hasher)
},
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ pub enum EvalErrorKind<'tcx, O> {
InvalidBool,
InvalidDiscriminant(u128),
PointerOutOfBounds {
ptr: Pointer,
offset: Size,
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocations don't know which AllocIds (there might be multiple) point to it. So if I have a method on an Allocation, I can't create an error containing an AllocId except by passing it as an argument just used for the error

Copy link
Member

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:

there might be multiple

Eh, what?!??

Copy link
Contributor Author

@oli-obk oli-obk Oct 23, 2018

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. :/

we can store the AllocId of an Allocation in the Allocation itself if you prefer that

Eh, what?!??

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

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating AllocIds for string literals will deduplicate the memory of the content, but still generate a new AllocId

access: bool,
allocation_size: Size,
},
Expand Down Expand Up @@ -440,10 +440,10 @@ impl<'tcx, O: fmt::Debug> fmt::Debug for EvalErrorKind<'tcx, O> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use self::EvalErrorKind::*;
match *self {
PointerOutOfBounds { ptr, access, allocation_size } => {
write!(f, "{} at offset {}, outside bounds of allocation {} which has size {}",
PointerOutOfBounds { offset, access, allocation_size } => {
write!(f, "{} at offset {}, outside bounds of allocation with size {}",
if access { "memory access" } else { "pointer computed" },
ptr.offset.bytes(), ptr.alloc_id, allocation_size.bytes())
offset.bytes(), allocation_size.bytes())
},
MemoryLockViolation { ptr, len, frame, access, ref lock } => {
write!(f, "{:?} access by frame {} at {:?}, size {}, is in conflict with lock {:?}",
Expand Down
206 changes: 196 additions & 10 deletions src/librustc/mir/interpret/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@ 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;
use std::io;
use std::ops::{Deref, DerefMut};
use std::hash::Hash;
use std::cmp;
use std::hash::{Hash, Hasher};
use syntax::ast::Mutability;
use rustc_serialize::{Encoder, Decodable, Encodable};
use rustc_data_structures::sorted_map::SortedMap;
Expand Down Expand Up @@ -217,9 +218,56 @@ impl<'tcx, Tag> Pointer<Tag> {
}


#[derive(Copy, Clone, Eq, Hash, Ord, PartialEq, PartialOrd, Debug)]
#[derive(Copy, Clone, Debug)]
pub struct AllocId(pub u64);

impl Eq for AllocId {}

impl Hash for AllocId {
fn hash<H: Hasher>(&self, state: &mut H) {
ty::tls::with(|tcx| {
let alloc_type = tcx.alloc_map.lock().get(*self);
match alloc_type {
Some(alloc_type) => alloc_type.hash(state),
None => self.0.hash(state),
}
})
}
}

impl PartialEq for AllocId {
fn eq(&self, other: &Self) -> bool {
ty::tls::with(|tcx| {
let (s, o) = {
let map = tcx.alloc_map.lock();
(map.get(*self), map.get(*other))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, wait, what? That doesn't seem right to me.

Certainly, ptr comparison in miri should compare the actual IDs, the the content that's stored behind these IDs. What is the type of s and o here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only applies to AllocIds that refer to global allocations. Comparing by id is inherently wrong. We never ran into any trouble, because we never used a ConstValue containing an AllocId in comparisons that mattered. Now that everything is ByRef, pattern match exhaustiveness checks failed because they couldn't figure out that two const patterns are the same if their AllocId differs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only applies to AllocIds that refer to global allocations.

It is still grossly wrong in the context of the miri engine. Ptr equality wants to compare the IDs, not the allocations they point to, and the IDs should be newtyped to not mix them up with other integers.

pattern match exhaustiveness checks failed because they couldn't figure out that two const patterns are the same if their AllocId differs.

Then it shouldn't use the same equality as the miri engine. If you want to overwrite equality and hashing for ConstValue, I can support that, but doing it for a type used inside the engine makes me really nervous.

};
s == o
})
}
}

impl Ord for AllocId {
fn cmp(&self, other: &Self) -> cmp::Ordering {
match self.partial_cmp(other) {
Some(ord) => ord,
None => self.0.cmp(&other.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it okay to fall back to comparing the integers here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens only for non-interned allocations

}
}
}

impl PartialOrd for AllocId {
fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
ty::tls::with(|tcx| {
let (s, o) = {
let map = tcx.alloc_map.lock();
(map.get(*self)?, map.get(*other)?)
};
s.partial_cmp(&o)
})
}
}

impl ::rustc_serialize::UseSpecializedEncodable for AllocId {}
impl ::rustc_serialize::UseSpecializedDecodable for AllocId {}

Expand Down Expand Up @@ -427,7 +475,7 @@ impl fmt::Display for AllocId {
}
}

#[derive(Debug, Clone, Eq, PartialEq, Hash, RustcDecodable, RustcEncodable)]
#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, RustcDecodable, RustcEncodable)]
pub enum AllocType<'tcx, M> {
/// The alloc id is used as a function pointer
Function(Instance<'tcx>),
Expand All @@ -440,7 +488,7 @@ pub enum AllocType<'tcx, M> {

pub struct AllocMap<'tcx, M> {
/// Lets you know what an AllocId refers to
id_to_type: FxHashMap<AllocId, AllocType<'tcx, M>>,
id_to_type: FxHashMap<u64, AllocType<'tcx, M>>,
Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we lose the newtype protection for not mixing up IDs.

I don't understand the purpose of this entire commit, at all. It also suffers from an astute lack of comments :)


/// Used to ensure that functions and statics only get one associated AllocId
type_interner: FxHashMap<AllocType<'tcx, M>, AllocId>,
Expand Down Expand Up @@ -479,7 +527,7 @@ impl<'tcx, M: fmt::Debug + Eq + Hash + Clone> AllocMap<'tcx, M> {
}
let id = self.reserve();
debug!("creating alloc_type {:?} with id {}", alloc_type, id);
self.id_to_type.insert(id, alloc_type.clone());
self.id_to_type.insert(id.0, alloc_type.clone());
self.type_interner.insert(alloc_type, id);
id
}
Expand All @@ -492,7 +540,7 @@ impl<'tcx, M: fmt::Debug + Eq + Hash + Clone> AllocMap<'tcx, M> {
}

pub fn get(&self, id: AllocId) -> Option<AllocType<'tcx, M>> {
self.id_to_type.get(&id).cloned()
self.id_to_type.get(&id.0).cloned()
}

pub fn unwrap_memory(&self, id: AllocId) -> M {
Expand All @@ -513,13 +561,13 @@ impl<'tcx, M: fmt::Debug + Eq + Hash + Clone> AllocMap<'tcx, M> {
}

pub fn set_id_memory(&mut self, id: AllocId, mem: M) {
if let Some(old) = self.id_to_type.insert(id, AllocType::Memory(mem)) {
if let Some(old) = self.id_to_type.insert(id.0, AllocType::Memory(mem)) {
bug!("tried to set allocation id {}, but it was already existing as {:#?}", id, old);
}
}

pub fn set_id_same_memory(&mut self, id: AllocId, mem: M) {
self.id_to_type.insert_same(id, AllocType::Memory(mem));
self.id_to_type.insert_same(id.0, AllocType::Memory(mem));
}
}

Expand All @@ -545,7 +593,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);
Expand Down Expand Up @@ -575,6 +623,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() {
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(
Copy link
Member

Choose a reason for hiding this comment

The 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;
Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if this overflows? (Same below in check_defined.)

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(
Copy link
Member

Choose a reason for hiding this comment

The 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 memory.rs, please don't drop these :)

&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(
Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

The 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 memory.rs uses this in copy, it is required to provide an address stability guarantee on its return value.

&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>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why does get_bytes return a Scalar<Tag>? That doesn't make any sense, a method of that name should return a &[u8]. Now I am even more puzzled about what it does.

(See, a doc comment would have been useful. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is called read_scalar?

Copy link
Member

@RalfJung RalfJung Oct 23, 2018

Choose a reason for hiding this comment

The 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] {
Copy link
Member

Choose a reason for hiding this comment

The 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 {}
Expand Down
Loading