rewrite handle impl again

6 files changed, 90 insertions, 88 deletions

diff --git a/src/shims/windows/dlsym.rs b/src/shims/windows/dlsym.rs
index d64be9cc052..fc36913638e 100644
--- a/src/shims/windows/dlsym.rs
+++ b/src/shims/windows/dlsym.rs
@@ -5,7 +5,7 @@ use rustc_target::spec::abi::Abi;
 use log::trace;
 
 use crate::helpers::check_arg_count;
-use crate::shims::windows::handle::{EvalContextExt as _, Handle};
+use crate::shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle};
 use crate::*;
 
 #[derive(Debug, Copy, Clone)]
@@ -112,14 +112,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             Dlsym::SetThreadDescription => {
                 let [handle, name] = check_arg_count(args)?;
 
+                let handle = this.read_scalar(handle)?.check_init()?;
+
                 let name = this.read_wide_str(this.read_pointer(name)?)?;
 
-                let thread =
-                    match Handle::from_scalar(this.read_scalar(handle)?.check_init()?, this)? {
-                        Some(Handle::Thread(thread)) => thread,
-                        Some(Handle::CurrentThread) => this.get_active_thread(),
-                        _ => this.invalid_handle("SetThreadDescription")?,
-                    };
+                let thread = match Handle::from_scalar(handle, this)? {
+                    Some(Handle::Thread(thread)) => thread,
+                    Some(Handle::Pseudo(PseudoHandle::CurrentThread)) => this.get_active_thread(),
+                    _ => this.invalid_handle("SetThreadDescription")?,
+                };
 
                 this.set_thread_name_wide(thread, name);
 
diff --git a/src/shims/windows/foreign_items.rs b/src/shims/windows/foreign_items.rs
index 6014281100f..cc030ec3d0c 100644
--- a/src/shims/windows/foreign_items.rs
+++ b/src/shims/windows/foreign_items.rs
@@ -1,14 +1,12 @@
 use std::iter;
-use std::time::{Duration, Instant};
 
 use rustc_span::Symbol;
 use rustc_target::abi::Size;
 use rustc_target::spec::abi::Abi;
 
-use crate::thread::Time;
 use crate::*;
 use shims::foreign_items::EmulateByNameResult;
-use shims::windows::handle::{EvalContextExt as _, Handle};
+use shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle};
 use shims::windows::sync::EvalContextExt as _;
 use shims::windows::thread::EvalContextExt as _;
 
@@ -373,7 +371,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             "GetCurrentThread" => {
                 let [] = this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
 
-                this.write_scalar(Handle::CurrentThread.to_scalar(this), dest)?;
+                this.write_scalar(
+                    Handle::Pseudo(PseudoHandle::CurrentThread).to_scalar(this),
+                    dest,
+                )?;
             }
 
             // Incomplete shims that we "stub out" just to get pre-main initialization code to work.
diff --git a/src/shims/windows/handle.rs b/src/shims/windows/handle.rs
index e1617ae6a8d..041033717e4 100644
--- a/src/shims/windows/handle.rs
+++ b/src/shims/windows/handle.rs
@@ -3,44 +3,79 @@ use std::mem::variant_count;
 
 use crate::*;
 
-/// A Windows `HANDLE` that represents a resource instead of being null or a pseudohandle.
-///
-/// This is a seperate type from [`Handle`] to simplify the packing and unpacking code.
-#[derive(Clone, Copy)]
-enum RealHandle {
-    Thread(ThreadId),
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub enum PseudoHandle {
+    CurrentThread,
 }
 
-impl RealHandle {
-    const USABLE_BITS: u32 = 31;
+impl PseudoHandle {
+    const CURRENT_THREAD_VALUE: u32 = 0;
 
-    const THREAD_DISCRIMINANT: u32 = 1;
+    fn value(self) -> u32 {
+        match self {
+            Self::CurrentThread => Self::CURRENT_THREAD_VALUE,
+        }
+    }
+
+    fn from_value(value: u32) -> Option<Self> {
+        match value {
+            Self::CURRENT_THREAD_VALUE => Some(Self::CurrentThread),
+            _ => None,
+        }
+    }
+}
+
+/// Miri representation of a Windows `HANDLE`
+#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
+pub enum Handle {
+    Null,
+    Pseudo(PseudoHandle),
+    Thread(ThreadId),
+}
+
+impl Handle {
+    const NULL_DISCRIMINANT: u32 = 0;
+    const PSEUDO_DISCRIMINANT: u32 = 1;
+    const THREAD_DISCRIMINANT: u32 = 2;
 
     fn discriminant(self) -> u32 {
         match self {
-            // can't use zero here because all zero handle is invalid
+            Self::Null => Self::NULL_DISCRIMINANT,
+            Self::Pseudo(_) => Self::PSEUDO_DISCRIMINANT,
             Self::Thread(_) => Self::THREAD_DISCRIMINANT,
         }
     }
 
     fn data(self) -> u32 {
         match self {
+            Self::Null => 0,
+            Self::Pseudo(pseudo_handle) => pseudo_handle.value(),
             Self::Thread(thread) => thread.to_u32(),
         }
     }
 
     fn packed_disc_size() -> u32 {
-        // log2(x) + 1 is how many bits it takes to store x
-        // because the discriminants start at 1, the variant count is equal to the highest discriminant
-        variant_count::<Self>().ilog2() + 1
+        // ceil(log2(x)) is how many bits it takes to store x numbers
+        let variant_count = variant_count::<Self>();
+
+        // however, std's ilog2 is floor(log2(x))
+        let floor_log2 = variant_count.ilog2();
+
+        // we need to add one for non powers of two to compensate for the difference
+        let ceil_log2 = if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 };
+
+        ceil_log2
     }
 
-    /// This function packs the discriminant and data values into a 31-bit space.
+    /// Converts a handle into its machine representation.
+    ///
+    /// The upper [`Self::packed_disc_size()`] bits are used to store a discriminant corresponding to the handle variant.
+    /// The remaining bits are used for the variant's field.
+    ///
     /// None of this layout is guaranteed to applications by Windows or Miri.
-    /// The sign bit is not used to avoid overlapping any pseudo-handles.
-    fn to_packed(self) -> i32 {
+    fn to_packed(self) -> u32 {
         let disc_size = Self::packed_disc_size();
-        let data_size = Self::USABLE_BITS - disc_size;
+        let data_size = u32::BITS - disc_size;
 
         let discriminant = self.discriminant();
         let data = self.data();
@@ -53,90 +88,54 @@ impl RealHandle {
 
         // packs the data into the lower `data_size` bits
         // and packs the discriminant right above the data
-        (discriminant << data_size | data) as i32
+        discriminant << data_size | data
     }
 
     fn new(discriminant: u32, data: u32) -> Option<Self> {
         match discriminant {
+            Self::NULL_DISCRIMINANT if data == 0 => Some(Self::Null),
+            Self::PSEUDO_DISCRIMINANT => Some(Self::Pseudo(PseudoHandle::from_value(data)?)),
             Self::THREAD_DISCRIMINANT => Some(Self::Thread(data.into())),
             _ => None,
         }
     }
 
     /// see docs for `to_packed`
-    fn from_packed(handle: i32) -> Option<Self> {
-        let handle_bits = handle as u32;
-
+    fn from_packed(handle: u32) -> Option<Self> {
         let disc_size = Self::packed_disc_size();
-        let data_size = Self::USABLE_BITS - disc_size;
+        let data_size = u32::BITS - disc_size;
 
         // the lower `data_size` bits of this mask are 1
         let data_mask = 2u32.pow(data_size) - 1;
 
         // the discriminant is stored right above the lower `data_size` bits
-        let discriminant = handle_bits >> data_size;
+        let discriminant = handle >> data_size;
 
         // the data is stored in the lower `data_size` bits
-        let data = handle_bits & data_mask;
+        let data = handle & data_mask;
 
         Self::new(discriminant, data)
     }
-}
-
-/// Miri representation of a Windows `HANDLE`
-#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
-pub enum Handle {
-    Null, // = 0
-
-    // pseudo-handles
-    // The lowest real windows pseudo-handle is -6, so miri pseduo-handles start at -7 to break code hardcoding these values
-    CurrentThread, // = -7
-
-    // real handles
-    Thread(ThreadId),
-}
-
-impl Handle {
-    const CURRENT_THREAD_VALUE: i32 = -7;
-
-    fn to_packed(self) -> i32 {
-        match self {
-            Self::Null => 0,
-            Self::CurrentThread => Self::CURRENT_THREAD_VALUE,
-            Self::Thread(thread) => RealHandle::Thread(thread).to_packed(),
-        }
-    }
 
     pub fn to_scalar(self, cx: &impl HasDataLayout) -> Scalar<Provenance> {
         // 64-bit handles are sign extended 32-bit handles
         // see https://docs.microsoft.com/en-us/windows/win32/winprog64/interprocess-communication
-        let handle = self.to_packed().into();
-
-        Scalar::from_machine_isize(handle, cx)
-    }
-
-    fn from_packed(handle: i64) -> Option<Self> {
-        let current_thread_val = Self::CURRENT_THREAD_VALUE as i64;
-
-        if handle == 0 {
-            Some(Self::Null)
-        } else if handle == current_thread_val {
-            Some(Self::CurrentThread)
-        } else if let Ok(handle) = handle.try_into() {
-            match RealHandle::from_packed(handle)? {
-                RealHandle::Thread(id) => Some(Self::Thread(id)),
-            }
-        } else {
-            // if a handle doesn't fit in an i32, it isn't valid.
-            None
-        }
+        let signed_handle = self.to_packed() as i32;
+        Scalar::from_machine_isize(signed_handle.into(), cx)
     }
 
     pub fn from_scalar<'tcx>(
         handle: Scalar<Provenance>,
         cx: &impl HasDataLayout,
     ) -> InterpResult<'tcx, Option<Self>> {
-        let handle = handle.to_machine_isize(cx)?;
+        let sign_extended_handle = handle.to_machine_isize(cx)?;
+
+        let handle = if let Ok(signed_handle) = i32::try_from(sign_extended_handle) {
+            signed_handle as u32
+        } else {
+            // if a handle doesn't fit in an i32, it isn't valid.
+            return Ok(None);
+        };
 
         Ok(Self::from_packed(handle))
     }
diff --git a/src/shims/windows/thread.rs b/src/shims/windows/thread.rs
index 6b6c4916dee..08eb4ddba10 100644
--- a/src/shims/windows/thread.rs
+++ b/src/shims/windows/thread.rs
@@ -2,7 +2,7 @@ use rustc_middle::ty::layout::LayoutOf;
 use rustc_target::spec::abi::Abi;
 
 use crate::*;
-use shims::windows::handle::{EvalContextExt as _, Handle};
+use shims::windows::handle::{EvalContextExt as _, Handle, PseudoHandle};
 
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 
@@ -58,7 +58,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             Some(Handle::Thread(thread)) => thread,
             // Unlike on posix, joining the current thread is not UB on windows.
             // It will just deadlock.
-            Some(Handle::CurrentThread) => this.get_active_thread(),
+            Some(Handle::Pseudo(PseudoHandle::CurrentThread)) => this.get_active_thread(),
             _ => this.invalid_handle("WaitForSingleObject")?,
         };
 
diff --git a/tests/fail/concurrency/windows_join_main.rs b/tests/fail/concurrency/windows_join_main.rs
index ea52220d449..d3b54cdf156 100644
--- a/tests/fail/concurrency/windows_join_main.rs
+++ b/tests/fail/concurrency/windows_join_main.rs
@@ -7,7 +7,7 @@
 use std::thread;
 
 extern "system" {
-    fn WaitForSingleObject(handle: usize, timeout: u32) -> u32;
+    fn WaitForSingleObject(handle: isize, timeout: u32) -> u32;
 }
 
 const INFINITE: u32 = u32::MAX;
@@ -15,7 +15,7 @@ const INFINITE: u32 = u32::MAX;
 // This is how miri represents the handle for thread 0.
 // This value can be "legitimately" obtained by using `GetCurrentThread` with `DuplicateHandle`
 // but miri does not implement `DuplicateHandle` yet.
-const MAIN_THREAD: usize = 1 << 30;
+const MAIN_THREAD: isize = (2i32 << 30) as isize;
 
 fn main() {
     thread::spawn(|| {
diff --git a/tests/fail/concurrency/windows_join_main.stderr b/tests/fail/concurrency/windows_join_main.stderr
index 72b854d354a..ff0d074fa7d 100644
--- a/tests/fail/concurrency/windows_join_main.stderr
+++ b/tests/fail/concurrency/windows_join_main.stderr
@@ -1,10 +1,11 @@
 error: deadlock: the evaluated program deadlocked
   --> $DIR/windows_join_main.rs:LL:CC
    |
-LL |         WaitForSingleObject(MAIN_THREAD, INFINITE);
-   |                                                   ^ the evaluated program deadlocked
+LL |             assert_eq!(WaitForSingleObject(MAIN_THREAD, INFINITE), 0);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program deadlocked
    |
-   = note: inside closure at $DIR/windows_join_main.rs:LL:CC
+   = note: inside closure at RUSTLIB/core/src/macros/mod.rs:LL:CC
+   = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace