Most review comments - still missing shim tests

src/shims/windows/fs.rs

@@ -1,8 +1,7 @@
-use std::borrow::Cow;
 use std::fs::{File, Metadata, OpenOptions};
 use std::io;
 use std::io::{IsTerminal, Read, Seek, SeekFrom, Write};
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::time::SystemTime;
 
 use rustc_abi::Size;
@@ -116,6 +115,14 @@ impl FileDescription for DirHandle {
     fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result<Metadata>> {
         interp_ok(self.path.metadata())
     }
+
+    fn close<'tcx>(
+        self: Box<Self>,
+        _communicate_allowed: bool,
+        _ecx: &mut MiriInterpCx<'tcx>,
+    ) -> InterpResult<'tcx, io::Result<()>> {
+        interp_ok(Ok(()))
+    }
 }
 
 #[derive(Debug)]
@@ -131,6 +138,14 @@ impl FileDescription for MetadataHandle {
     fn metadata<'tcx>(&self) -> InterpResult<'tcx, io::Result<Metadata>> {
         interp_ok(self.path.metadata())
     }
+
+    fn close<'tcx>(
+        self: Box<Self>,
+        _communicate_allowed: bool,
+        _ecx: &mut MiriInterpCx<'tcx>,
+    ) -> InterpResult<'tcx, io::Result<()>> {
+        interp_ok(Ok(()))
+    }
 }
 
 impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
@@ -151,9 +166,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         this.assert_target_os("windows", "CreateFileW");
         this.check_no_isolation("`CreateFileW`")?;
 
-        let file_name =
-            String::from_utf16_lossy(&this.read_wide_str(this.read_pointer(file_name)?)?);
-        let file_name = local_path(&file_name);
+        let file_name = this.read_path_from_wide_str(this.read_pointer(file_name)?)?;
         let desired_access = this.read_scalar(desired_access)?.to_u32()?;
         let share_mode = this.read_scalar(share_mode)?.to_u32()?;
         let security_attributes = this.read_pointer(security_attributes)?;
@@ -267,19 +280,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let handle = if is_dir && exists {
             let fh = &mut this.machine.fds;
-            let fd = fh.insert_new(DirHandle { path: file_name.into() });
-            Ok(Handle::File(fd))
+            let fd_num = fh.insert_new(DirHandle { path: file_name.into() });
+            Ok(Handle::File(fd_num))
         } else if creation_disposition == open_existing && desired_access == 0 {
             // Windows supports handles with no permissions. These allow things such as reading
             // metadata, but not file content.
             let fh = &mut this.machine.fds;
-            let fd = fh.insert_new(MetadataHandle { path: file_name.into() });
-            Ok(Handle::File(fd))
+            let fd_num = fh.insert_new(MetadataHandle { path: file_name.into() });
+            Ok(Handle::File(fd_num))
         } else {
             options.open(file_name).map(|file| {
                 let fh = &mut this.machine.fds;
-                let fd = fh.insert_new(FileHandle { file, writable: desired_write });
-                Handle::File(fd)
+                let fd_num = fh.insert_new(FileHandle { file, writable: desired_write });
+                Handle::File(fd_num)
             })
         };
 
@@ -308,13 +321,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             this.windows_ty_layout("BY_HANDLE_FILE_INFORMATION"),
         )?;
 
-        let fd = if let Handle::File(fd) = file {
-            fd
+        let fd_num = if let Handle::File(fd_num) = file {
+            fd_num
         } else {
             this.invalid_handle("GetFileInformationByHandle")?
         };
 
-        let Some(desc) = this.machine.fds.get(fd) else {
+        let Some(desc) = this.machine.fds.get(fd_num) else {
             this.invalid_handle("GetFileInformationByHandle")?
         };
 
@@ -391,13 +404,3 @@ fn write_filetime_field<'tcx>(
         &cx.project_field_named(val, name)?,
     )
 }
-
-fn local_path(path: &str) -> Cow<'_, Path> {
-    if cfg!(windows) {
-        Cow::Borrowed(path.as_ref())
-    } else {
-        let stripped = if path.starts_with(r"\\?\") { &path[3..] } else { path };
-        let path = PathBuf::from(stripped.replace("\\", "/"));
-        Cow::Owned(path)
-    }
-}

src/shims/windows/handle.rs

@@ -6,6 +6,9 @@ use rustc_abi::HasDataLayout;
 use crate::concurrency::thread::ThreadNotFound;
 use crate::*;
 
+/// Internal type of a file-descriptor - this is what [`FdTable`](shims::files::FdTable) expects
+type FdNum = i32;
+
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 pub enum PseudoHandle {
     CurrentThread,
@@ -17,7 +20,7 @@ pub enum Handle {
     Null,
     Pseudo(PseudoHandle),
     Thread(ThreadId),
-    File(i32),
+    File(FdNum),
     Invalid,
 }
 
@@ -51,6 +54,8 @@ impl Handle {
     const PSEUDO_DISCRIMINANT: u32 = 1;
     const THREAD_DISCRIMINANT: u32 = 2;
     const FILE_DISCRIMINANT: u32 = 3;
+    // Chosen to ensure Handle::Invalid encodes to -1. Update this value if there are ever more than
+    // 8 discriminants.
     const INVALID_DISCRIMINANT: u32 = 7;
 
     fn discriminant(self) -> u32 {
@@ -70,20 +75,25 @@ impl Handle {
             Self::Thread(thread) => thread.to_u32(),
             #[expect(clippy::cast_sign_loss)]
             Self::File(fd) => fd as u32,
+            // INVALID_HANDLE_VALUE is -1. This fact is explicitly declared or implied in several
+            // pages of Windows documentation.
+            // 1: https://learn.microsoft.com/en-us/dotnet/api/microsoft.win32.safehandles.safefilehandle?view=net-9.0
+            // 2: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=msvc-170
             Self::Invalid => 0x1FFFFFFF,
         }
     }
 
     fn packed_disc_size() -> u32 {
-        // ceil(log2(x)) is how many bits it takes to store x numbers
-        // We ensure that INVALID_HANDLE_VALUE (0xFFFFFFFF) decodes to Handle::Invalid
-        // see https://devblogs.microsoft.com/oldnewthing/20230914-00/?p=108766
+        // ceil(log2(x)) is how many bits it takes to store x numbers.
+        // We ensure that INVALID_HANDLE_VALUE (0xFFFFFFFF) decodes to Handle::Invalid.
+        // see https://devblogs.microsoft.com/oldnewthing/20230914-00/?p=108766 for more detail on
+        // INVALID_HANDLE_VALUE.
         let variant_count = variant_count::<Self>();
 
-        // however, std's ilog2 is floor(log2(x))
+        // 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
+        // We need to add one for non powers of two to compensate for the difference.
         #[expect(clippy::arithmetic_side_effects)] // cannot overflow
         if variant_count.is_power_of_two() { floor_log2 } else { floor_log2 + 1 }
     }
@@ -105,7 +115,7 @@ impl Handle {
         assert!(discriminant < 2u32.pow(disc_size));
 
         // make sure the data fits into `data_size` bits
-        assert!(data <= 2u32.pow(data_size));
+        assert!(data < 2u32.pow(data_size));
 
         // packs the data into the lower `data_size` bits
         // and packs the discriminant right above the data
@@ -118,7 +128,11 @@ impl Handle {
             Self::PSEUDO_DISCRIMINANT => Some(Self::Pseudo(PseudoHandle::from_value(data)?)),
             Self::THREAD_DISCRIMINANT => Some(Self::Thread(ThreadId::new_unchecked(data))),
             #[expect(clippy::cast_possible_wrap)]
-            Self::FILE_DISCRIMINANT => Some(Self::File(data as i32)),
+            Self::FILE_DISCRIMINANT => {
+                // This cast preserves all bits.
+                assert_eq!(size_of_val(&data), size_of::<FdNum>());
+                Some(Self::File(data as FdNum))
+            }
             Self::INVALID_DISCRIMINANT => Some(Self::Invalid),
             _ => None,
         }
@@ -221,9 +235,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 this.detach_thread(thread, /*allow_terminated_joined*/ true)?;
                 this.eval_windows("c", "TRUE")
             }
-            Handle::File(fd) =>
-                if let Some(file) = this.machine.fds.get(fd) {
-                    let err = file.close(this.machine.communicate(), this)?;
+            Handle::File(fd_num) =>
+                if let Some(fd) = this.machine.fds.remove(fd_num) {
+                    let err = fd.close(this.machine.communicate(), this)?;
                     if let Err(e) = err {
                         this.set_last_error(e)?;
                         this.eval_windows("c", "FALSE")