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

ensure that integers cast to pointers will never point at a valid alloc, not even the zst alloc #81

Merged
merged 19 commits into from
Nov 17, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
20 changes: 9 additions & 11 deletions src/interpreter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,6 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
PrimVal::from_uint_with_size(n, self.memory.pointer_size())
}

fn isize_primval(&self, n: i64) -> PrimVal {
PrimVal::from_int_with_size(n, self.memory.pointer_size())
}

fn str_to_value(&mut self, s: &str) -> EvalResult<'tcx, Value> {
// FIXME: cache these allocs
let ptr = self.memory.allocate(s.len(), 1)?;
Expand Down Expand Up @@ -523,7 +519,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
self.write_value(value, dest, value_ty)?;
} else {
assert_eq!(operands.len(), 0);
let zero = self.isize_primval(0);
let value_size = self.type_size(dest_ty).expect("pointer types are sized");
let zero = PrimVal::from_int_with_size(0, value_size);
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? I think we can assume isize is pointer sized, and isize_primval does mean the target isize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want an isize here. The rawnullablepointer opt also happens for nonpointers

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, right, for NonZero things that aren't pointer-sized? I hate the RawNullablePointer names in layout...

self.write_primval(dest, zero)?;
}
} else {
Expand All @@ -541,13 +538,14 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
let operand_ty = self.operand_ty(operand);
assert_eq!(self.type_size(operand_ty), Some(0));
}
let offset = self.nonnull_offset(dest_ty, nndiscr, discrfield)?;
let (offset, ty) = self.nonnull_offset_and_ty(dest_ty, nndiscr, discrfield)?;

// FIXME(solson)
let dest = self.force_allocation(dest)?.to_ptr();

let dest = dest.offset(offset.bytes() as isize);
try!(self.memory.write_isize(dest, 0));
let dest_size = self.type_size(ty).unwrap_or(self.memory.pointer_size());
Copy link
Member

Choose a reason for hiding this comment

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

In what cases can this default to self.memory.pointer_size()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the unsized case. Since any unsized value is a pointer to the actual sized value and this optimization only cares about the pointer part, not the extra part

Copy link
Member

@solson solson Nov 13, 2016

Choose a reason for hiding this comment

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

What unsized case? ty is the type of the non-zero field that is used for the discriminant in this optimization, right? How can that field be unsized?

EDIT: Nevermind, I now see the conversation lower down; let's continue there.

try!(self.memory.write_int(dest, 0, dest_size));
}
} else {
bug!("tried to assign {:?} to Layout::RawNullablePointer", kind);
Expand Down Expand Up @@ -694,7 +692,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
}
}

fn nonnull_offset(&self, ty: Ty<'tcx>, nndiscr: u64, discrfield: &[u32]) -> EvalResult<'tcx, Size> {
fn nonnull_offset_and_ty(&self, ty: Ty<'tcx>, nndiscr: u64, discrfield: &[u32]) -> EvalResult<'tcx, (Size, Ty<'tcx>)> {
// Skip the constant 0 at the start meant for LLVM GEP.
let mut path = discrfield.iter().skip(1).map(|&i| i as usize);

Expand All @@ -709,10 +707,10 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
_ => bug!("non-enum for StructWrappedNullablePointer: {}", ty),
};

self.field_path_offset(inner_ty, path)
self.field_path_offset_and_ty(inner_ty, path)
}

fn field_path_offset<I: Iterator<Item = usize>>(&self, mut ty: Ty<'tcx>, path: I) -> EvalResult<'tcx, Size> {
fn field_path_offset_and_ty<I: Iterator<Item = usize>>(&self, mut ty: Ty<'tcx>, path: I) -> EvalResult<'tcx, (Size, Ty<'tcx>)> {
let mut offset = Size::from_bytes(0);

// Skip the initial 0 intended for LLVM GEP.
Expand All @@ -722,7 +720,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
offset = offset.checked_add(field_offset, &self.tcx.data_layout).unwrap();
}

Ok(offset)
Ok((offset, ty))
}

fn get_field_ty(&self, ty: Ty<'tcx>, field_index: usize) -> EvalResult<'tcx, Ty<'tcx>> {
Expand Down
15 changes: 9 additions & 6 deletions src/interpreter/terminator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,17 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
self.memory.read_int(adt_ptr, discr_size as usize)? as u64
}

RawNullablePointer { nndiscr, .. } => {
self.read_nonnull_discriminant_value(adt_ptr, nndiscr)?
RawNullablePointer { nndiscr, value } => {
let discr_size = value.size(&self.tcx.data_layout).bytes() as usize;
self.read_nonnull_discriminant_value(adt_ptr, nndiscr, discr_size)?
}

StructWrappedNullablePointer { nndiscr, ref discrfield, .. } => {
let offset = self.nonnull_offset(adt_ty, nndiscr, discrfield)?;
let (offset, ty) = self.nonnull_offset_and_ty(adt_ty, nndiscr, discrfield)?;
let nonnull = adt_ptr.offset(offset.bytes() as isize);
self.read_nonnull_discriminant_value(nonnull, nndiscr)?
// only the pointer part of a fat pointer is used for this space optimization
let discr_size = self.type_size(ty).unwrap_or(self.memory.pointer_size());
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here. When can the nonnull field be a unsized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be, but the MIR ends up giving us an index path that will give us the first field of the fat pointer, which ends up being te unsized value. Not sure if the code giving us the type path is wrong or if this is intended. I'll check and get back to you

Copy link
Member

Choose a reason for hiding this comment

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

The first field of the fat pointer is the thin data pointer. This is a GEP field path, it will not deref nested pointers, just offset into a flat structure.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. It sounds like we'd need to adjust the type, then, to match @eddyb's point. Alternately, maybe it can return the size instead of the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

self.read_nonnull_discriminant_value(nonnull, nndiscr, discr_size)?
}

// The discriminant_value intrinsic returns 0 for non-sum types.
Expand All @@ -281,8 +284,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
Ok(discr_val)
}

fn read_nonnull_discriminant_value(&self, ptr: Pointer, nndiscr: u64) -> EvalResult<'tcx, u64> {
let not_null = match self.memory.read_usize(ptr) {
fn read_nonnull_discriminant_value(&self, ptr: Pointer, nndiscr: u64, discr_size: usize) -> EvalResult<'tcx, u64> {
let not_null = match self.memory.read_uint(ptr, discr_size) {
Ok(0) => false,
Ok(_) | Err(EvalError::ReadPointerAsBytes) => true,
Err(e) => return Err(e),
Expand Down
14 changes: 14 additions & 0 deletions tests/run-pass/small_enum_size_bug.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![allow(dead_code)]

enum E {
A = 1,
B = 2,
C = 3,
}

fn main() {
let enone = None::<E>;
if let Some(..) = enone {
panic!();
}
}