Auto merge of #825 - RalfJung:no-null, r=RalfJung

bors · bors · commit 16d791bf8876 · 2019-07-05T19:25:10.000Z
avoid Scalar::is_null_ptr, it is going away

Comparing pointers should be done more carefully than that
diff --git a/src/helpers.rs b/src/helpers.rs
@@ -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(
diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs
@@ -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,
@@ -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 {
@@ -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()
@@ -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'=') {
@@ -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.
@@ -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);
                 }
@@ -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)?;
@@ -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
@@ -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)?;
@@ -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>>> {
diff --git a/src/shims/tls.rs b/src/shims/tls.rs
@@ -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>>,
 }
 
@@ -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,
             },
         );
@@ -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);
@@ -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::*;
 
@@ -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;
                 }
             }
@@ -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)?;
@@ -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`.
