Skip to content

Commit

Permalink
Auto merge of #825 - RalfJung:no-null, r=RalfJung
Browse files Browse the repository at this point in the history 
avoid Scalar::is_null_ptr, it is going away

Comparing pointers should be done more carefully than that
  • Loading branch information
@bors
bors committed Jul 5, 2019
2 parents 3525943 + 698b311 commit 16d791b
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 35 deletions.
22 changes: 22 additions & 0 deletions src/helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
})
}

/// Write a 0 of the appropriate size to `dest`.
fn write_null(&mut self, dest: PlaceTy<'tcx, Tag>) -> InterpResult<'tcx> {
self.eval_context_mut().write_scalar(Scalar::from_int(0, dest.layout.size), dest)
}

/// Test if this immediate equals 0.
fn is_null(&self, val: Scalar<Tag>) -> InterpResult<'tcx, bool> {
let this = self.eval_context_ref();
let null = Scalar::from_int(0, this.memory().pointer_size());
this.ptr_eq(val, null)
}

/// Turn a Scalar into an Option<NonNullScalar>
fn test_null(&self, val: Scalar<Tag>) -> InterpResult<'tcx, Option<Scalar<Tag>>> {
let this = self.eval_context_ref();
Ok(if this.is_null(val)? {
None
} else {
Some(val)
})
}

/// Visits the memory covered by `place`, sensitive to freezing: the 3rd parameter
/// will be true if this is frozen, false if this is in an `UnsafeCell`.
fn visit_freeze_sensitive(
Expand Down
35 changes: 13 additions & 22 deletions src/shims/foreign_items.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
ptr: Scalar<Tag>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
if !ptr.is_null_ptr(this) {
if !this.is_null(ptr)? {
this.memory_mut().deallocate(
ptr.to_ptr()?,
None,
Expand All @@ -66,7 +66,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
) -> InterpResult<'tcx, Scalar<Tag>> {
let this = self.eval_context_mut();
let align = this.min_align();
if old_ptr.is_null_ptr(this) {
if this.is_null(old_ptr)? {
if new_size == 0 {
Ok(Scalar::from_int(0, this.pointer_size()))
} else {
Expand Down Expand Up @@ -427,7 +427,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let mut success = None;
{
let name_ptr = this.read_scalar(args[0])?.not_undef()?;
if !name_ptr.is_null_ptr(this) {
if !this.is_null(name_ptr)? {
let name_ptr = name_ptr.to_ptr()?;
let name = this
.memory()
Expand Down Expand Up @@ -455,7 +455,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let name_ptr = this.read_scalar(args[0])?.not_undef()?;
let value_ptr = this.read_scalar(args[1])?.to_ptr()?;
let value = this.memory().get(value_ptr.alloc_id)?.read_c_str(tcx, value_ptr)?;
if !name_ptr.is_null_ptr(this) {
if !this.is_null(name_ptr)? {
let name_ptr = name_ptr.to_ptr()?;
let name = this.memory().get(name_ptr.alloc_id)?.read_c_str(tcx, name_ptr)?;
if !name.is_empty() && !name.contains(&b'=') {
Expand Down Expand Up @@ -638,14 +638,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let key_ptr = this.read_scalar(args[0])?.not_undef()?;

// Extract the function type out of the signature (that seems easier than constructing it ourselves).
let dtor = match this.read_scalar(args[1])?.not_undef()? {
Scalar::Ptr(dtor_ptr) => Some(this.memory().get_fn(dtor_ptr)?),
Scalar::Raw { data: 0, size } => {
// NULL pointer
assert_eq!(size as u64, this.memory().pointer_size().bytes());
None
},
Scalar::Raw { .. } => return err!(ReadBytesAsPointer),
let dtor = match this.test_null(this.read_scalar(args[1])?.not_undef()?)? {
Some(dtor_ptr) => Some(this.memory().get_fn(dtor_ptr.to_ptr()?)?),
None => None,
};

// Figure out how large a pthread TLS key actually is.
Expand All @@ -657,7 +652,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
let key_layout = this.layout_of(key_type)?;

// Create key and write it into the memory where `key_ptr` wants it.
let key = this.machine.tls.create_tls_key(dtor, tcx) as u128;
let key = this.machine.tls.create_tls_key(dtor) as u128;
if key_layout.size.bits() < 128 && key >= (1u128 << key_layout.size.bits() as u128) {
return err!(OutOfTls);
}
Expand All @@ -682,13 +677,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
}
"pthread_getspecific" => {
let key = this.read_scalar(args[0])?.to_bits(args[0].layout.size)?;
let ptr = this.machine.tls.load_tls(key)?;
let ptr = this.machine.tls.load_tls(key, tcx)?;
this.write_scalar(ptr, dest)?;
}
"pthread_setspecific" => {
let key = this.read_scalar(args[0])?.to_bits(args[0].layout.size)?;
let new_ptr = this.read_scalar(args[1])?.not_undef()?;
this.machine.tls.store_tls(key, new_ptr)?;
this.machine.tls.store_tls(key, this.test_null(new_ptr)?)?;

// Return success (`0`).
this.write_null(dest)?;
Expand Down Expand Up @@ -842,7 +837,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
// This just creates a key; Windows does not natively support TLS destructors.

// Create key and return it.
let key = this.machine.tls.create_tls_key(None, tcx) as u128;
let key = this.machine.tls.create_tls_key(None) as u128;

// Figure out how large a TLS key actually is. This is `c::DWORD`.
if dest.layout.size.bits() < 128
Expand All @@ -853,13 +848,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
}
"TlsGetValue" => {
let key = this.read_scalar(args[0])?.to_u32()? as u128;
let ptr = this.machine.tls.load_tls(key)?;
let ptr = this.machine.tls.load_tls(key, tcx)?;
this.write_scalar(ptr, dest)?;
}
"TlsSetValue" => {
let key = this.read_scalar(args[0])?.to_u32()? as u128;
let new_ptr = this.read_scalar(args[1])?.not_undef()?;
this.machine.tls.store_tls(key, new_ptr)?;
this.machine.tls.store_tls(key, this.test_null(new_ptr)?)?;

// Return success (`1`).
this.write_scalar(Scalar::from_int(1, dest.layout.size), dest)?;
Expand Down Expand Up @@ -936,10 +931,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
Ok(())
}

fn write_null(&mut self, dest: PlaceTy<'tcx, Tag>) -> InterpResult<'tcx> {
self.eval_context_mut().write_scalar(Scalar::from_int(0, dest.layout.size), dest)
}

/// Evaluates the scalar at the specified path. Returns Some(val)
/// if the path could be resolved, and None otherwise
fn eval_path_scalar(&mut self, path: &[&str]) -> InterpResult<'tcx, Option<ScalarMaybeUndef<Tag>>> {
Expand Down
33 changes: 20 additions & 13 deletions src/shims/tls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@ use rustc::{ty, ty::layout::HasDataLayout, mir};
use crate::{
InterpResult, InterpError, StackPopCleanup,
MPlaceTy, Scalar, Tag,
HelpersEvalContextExt,
};

pub type TlsKey = u128;

#[derive(Copy, Clone, Debug)]
pub struct TlsEntry<'tcx> {
pub(crate) data: Scalar<Tag>, // Will eventually become a map from thread IDs to `Scalar`s, if we ever support more than one thread.
/// The data for this key. None is used to represent NULL.
/// (We normalize this early to avoid having to do a NULL-ptr-test each time we access the data.)
/// Will eventually become a map from thread IDs to `Scalar`s, if we ever support more than one thread.
pub(crate) data: Option<Scalar<Tag>>,
pub(crate) dtor: Option<ty::Instance<'tcx>>,
}

Expand All @@ -38,14 +42,13 @@ impl<'tcx> TlsData<'tcx> {
pub fn create_tls_key(
&mut self,
dtor: Option<ty::Instance<'tcx>>,
cx: &impl HasDataLayout,
) -> TlsKey {
let new_key = self.next_key;
self.next_key += 1;
self.keys.insert(
new_key,
TlsEntry {
data: Scalar::ptr_null(cx).into(),
data: None,
dtor,
},
);
Expand All @@ -63,17 +66,21 @@ impl<'tcx> TlsData<'tcx> {
}
}

pub fn load_tls(&mut self, key: TlsKey) -> InterpResult<'tcx, Scalar<Tag>> {
pub fn load_tls(
&mut self,
key: TlsKey,
cx: &impl HasDataLayout,
) -> InterpResult<'tcx, Scalar<Tag>> {
match self.keys.get(&key) {
Some(&TlsEntry { data, .. }) => {
trace!("TLS key {} loaded: {:?}", key, data);
Ok(data)
Ok(data.unwrap_or_else(|| Scalar::ptr_null(cx).into()))
}
None => err!(TlsOutOfBounds),
}
}

pub fn store_tls(&mut self, key: TlsKey, new_data: Scalar<Tag>) -> InterpResult<'tcx> {
pub fn store_tls(&mut self, key: TlsKey, new_data: Option<Scalar<Tag>>) -> InterpResult<'tcx> {
match self.keys.get_mut(&key) {
Some(&mut TlsEntry { ref mut data, .. }) => {
trace!("TLS key {} stored: {:?}", key, new_data);
Expand Down Expand Up @@ -105,7 +112,6 @@ impl<'tcx> TlsData<'tcx> {
fn fetch_tls_dtor(
&mut self,
key: Option<TlsKey>,
cx: &impl HasDataLayout,
) -> Option<(ty::Instance<'tcx>, Scalar<Tag>, TlsKey)> {
use std::collections::Bound::*;

Expand All @@ -117,10 +123,10 @@ impl<'tcx> TlsData<'tcx> {
for (&key, &mut TlsEntry { ref mut data, dtor }) in
thread_local.range_mut((start, Unbounded))
{
if !data.is_null_ptr(cx) {
if let Some(data_scalar) = *data {
if let Some(dtor) = dtor {
let ret = Some((dtor, *data, key));
*data = Scalar::ptr_null(cx);
let ret = Some((dtor, data_scalar, key));
*data = None;
return ret;
}
}
Expand All @@ -133,10 +139,11 @@ impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tc
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
fn run_tls_dtors(&mut self) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let mut dtor = this.machine.tls.fetch_tls_dtor(None, &*this.tcx);
let mut dtor = this.machine.tls.fetch_tls_dtor(None);
// FIXME: replace loop by some structure that works with stepping
while let Some((instance, ptr, key)) = dtor {
trace!("Running TLS dtor {:?} on {:?}", instance, ptr);
assert!(!this.is_null(ptr).unwrap(), "Data can't be NULL when dtor is called!");
// TODO: Potentially, this has to support all the other possible instances?
// See eval_fn_call in interpret/terminator/mod.rs
let mir = this.load_mir(instance.def)?;
Expand All @@ -157,9 +164,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
// step until out of stackframes
this.run()?;

dtor = match this.machine.tls.fetch_tls_dtor(Some(key), &*this.tcx) {
dtor = match this.machine.tls.fetch_tls_dtor(Some(key)) {
dtor @ Some(_) => dtor,
None => this.machine.tls.fetch_tls_dtor(None, &*this.tcx),
None => this.machine.tls.fetch_tls_dtor(None),
};
}
// FIXME: On a windows target, call `unsafe extern "system" fn on_tls_callback`.
Expand Down

0 comments on commit 16d791b

Please sign in to comment.