Convert Windows to use check_shim_sig instead of check_shim_sig_lenient #4779

CraftSpider · 2025-12-19T18:28:43Z

No description provided.

rustbot · 2025-12-19T18:28:47Z

Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two.
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

CraftSpider · 2025-12-19T18:46:52Z

src/shims/sig.rs

            "*const _" => $this.machine.layouts.const_raw_ptr.ty,
            "*mut _" => $this.machine.layouts.mut_raw_ptr.ty,
            ty if let Some(libc_ty) = ty.strip_prefix("libc::") => $this.libc_ty_layout(libc_ty).ty,
+            ty if let Some(win_ty) = ty.strip_prefix("winapi::") =>


This is shorthand for a longer path used by the windows_ty_layout function, I thought this made usage a lot nicer and more readable.

Sounds good! Please document this in the doc comment for shim_sig!.

CraftSpider · 2025-12-19T18:47:32Z

tests/pass-dep/getrandom.rs

    getrandom_01::getrandom(&mut data).unwrap();

+    // TODO: getrandom 0.2 uses the wrong return type for BCrypteGenRandom
+    #[cfg(not(target_os = "windows"))]


0.2 uses u32 for the return type, not i32. We now check this and report UB. Is this worth reporting upstream for a backport?

There was a getrandom 0.2 release this year... so yeah you might as well report it upstream, but no reason to block this PR on that.

CraftSpider · 2025-12-19T18:48:37Z

src/shims/windows/foreign_items.rs

-        } else {
-            CanonAbi::C
-        };
+        // TODO: Use this in shim-sig instead of 'system'? Or is that good enough for ExternAbi?


Drawing attention to this - no tests are failing, but I need to double check whether this works, or if I need to make the shim_sig macro support dynamic ABIs.

"system" was added exactly to support the Windows API, so extern "system" really ought to work. Please just remove this comment.

CraftSpider · 2025-12-19T18:56:40Z

src/shims/windows/foreign_items.rs

-                let [token, buf, size] =
-                    this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                let [token, buf, size] = this.check_shim_sig(
+                    shim_sig!(extern "system" fn(winapi::HANDLE, *mut _, *mut _) -> winapi::BOOL),


General note - I preferred using fundamental types in most places, but I left a couple fundamental windows API types as their winapi::* variants, for clarity or because they have somewhat odd Rust representations.

BOOL: Actually an i32, which is unexpected to most not familiar with it.

HANDLE: This has switched between isize and *mut _ in the past, and many Windows shims actually treat it as its own special thing.

HLOCAL and HMODULE: Aliases for HANDLE, but with the implication they represent only certain types of handle.

FARPROC: Actually Option<extern "C" fn()>. Much easier to load the resolved layout of that, instead of trying to add Option and fn special-cased to shim_sig_arg.

I can swap some of these to their literal representations if preferred, it may be a bit more performant, but it also may be worth adding caching to the *_ty_layout functions.

I'd add that a HLOCAL is a pointer to memory allocated by LocalAlloc and HMODULE is a pointer to a module's base address. Neither of these are handles in the same sense as HANDLE is despite being defined as a type alias (which is why they can't be passed to normal handle functions).

Sounds good! I will note though that this cuts both ways -- if someone reads just the Miri code, it's quite confusing to see winapi::BOOL and then later i32 for the same argument. Not sure if there's much we can do about that.

CraftSpider · 2025-12-19T18:57:20Z

src/shims/windows/foreign_items.rs

-                let [_First, _Handler] =
-                    this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                let [first, handler] = this.check_shim_sig(
+                    shim_sig!(extern "system" fn(u32, *mut _) -> *mut _),


I standardized all the names. I can undo this if desired, it just seemed like a good drive-by since most stuff uses Rust-style names, not the Windows API names.

Fine for me; so far we just didn't have anyone care enough to try and standardize this code.

FWIW, we also have a mix of styles for the functions that implement these shims, if they are implemented in separate functions.

rustbot · 2025-12-19T20:31:32Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

RalfJung

Thanks! Nice to see the macro work at scale. :)
I will just trust that those signatures are correct based on the test passing; I have no intuition for Windows APIs so I'd have to check every one of them manually...

The only actually requested change here is a comment nit.

View changes since this review

RalfJung · 2025-12-23T11:14:31Z

src/shims/sig.rs

            "*const _" => $this.machine.layouts.const_raw_ptr.ty,
            "*mut _" => $this.machine.layouts.mut_raw_ptr.ty,
            ty if let Some(libc_ty) = ty.strip_prefix("libc::") => $this.libc_ty_layout(libc_ty).ty,
+            ty if let Some(win_ty) = ty.strip_prefix("winapi::") =>


Sounds good! Please document this in the doc comment for shim_sig!.

RalfJung · 2025-12-23T11:15:43Z

tests/pass-dep/getrandom.rs

    getrandom_01::getrandom(&mut data).unwrap();

+    // TODO: getrandom 0.2 uses the wrong return type for BCrypteGenRandom
+    #[cfg(not(target_os = "windows"))]


There was a getrandom 0.2 release this year... so yeah you might as well report it upstream, but no reason to block this PR on that.

RalfJung · 2025-12-23T11:15:53Z

tests/pass-dep/getrandom.rs

    #[cfg(not(target_os = "solaris"))]
    getrandom_01::getrandom(&mut data).unwrap();

+    // TODO: getrandom 0.2 uses the wrong return type for BCrypteGenRandom


Suggested change

// TODO: getrandom 0.2 uses the wrong return type for BCrypteGenRandom

// On Windows, getrandom 0.2 uses the wrong return type for BCrypteGenRandom.

RalfJung · 2025-12-23T11:17:21Z

src/shims/windows/foreign_items.rs

-        } else {
-            CanonAbi::C
-        };
+        // TODO: Use this in shim-sig instead of 'system'? Or is that good enough for ExternAbi?


"system" was added exactly to support the Windows API, so extern "system" really ought to work. Please just remove this comment.

RalfJung · 2025-12-23T11:19:18Z

src/shims/windows/foreign_items.rs

-                let [token, buf, size] =
-                    this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                let [token, buf, size] = this.check_shim_sig(
+                    shim_sig!(extern "system" fn(winapi::HANDLE, *mut _, *mut _) -> winapi::BOOL),


Sounds good! I will note though that this cuts both ways -- if someone reads just the Miri code, it's quite confusing to see winapi::BOOL and then later i32 for the same argument. Not sure if there's much we can do about that.

RalfJung · 2025-12-23T11:19:57Z

src/shims/windows/foreign_items.rs

        // See `fn emulate_foreign_item_inner` in `shims/foreign_items.rs` for the general pattern.

        // Windows API stubs.
        // HANDLE = isize


Suggested change

// HANDLE = isize

// HANDLE = *mut c_void (formerly: isize)

RalfJung · 2025-12-23T11:22:26Z

src/shims/windows/foreign_items.rs

-                let [file, distance_to_move, new_file_pointer, move_method] =
-                    this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                let [file, distance_to_move, new_file_pointer, move_method] = this.check_shim_sig(
+                    // i64 is actually a LARGE_INTEGER union of {u32, i32} and {i64}


And that becomes just i64 on the ABI level? Those types are not ABI-compatible under Rust rules... but I guess they are compatible on Windows targets so "it's fine".^^

RalfJung · 2025-12-23T11:24:39Z

src/shims/windows/foreign_items.rs

-                let [_First, _Handler] =
-                    this.check_shim_sig_lenient(abi, sys_conv, link_name, args)?;
+                let [first, handler] = this.check_shim_sig(
+                    shim_sig!(extern "system" fn(u32, *mut _) -> *mut _),


Fine for me; so far we just didn't have anyone care enough to try and standardize this code.

FWIW, we also have a mix of styles for the functions that implement these shims, if they are implemented in separate functions.

rustbot · 2025-12-23T11:26:47Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Dec 19, 2025

CraftSpider commented Dec 19, 2025

View reviewed changes

CraftSpider added 2 commits December 19, 2025 12:13

Convert Windows to use check_shim_sig instead of check_shim_sig_lenient

c0a027c

Fix new FlushFileBuffers function

9cebf4e

CraftSpider force-pushed the windows-shim-sig branch from 4824149 to 9cebf4e Compare December 19, 2025 20:31

Fix return type of _Unwind_RaiseException

25047ae

RalfJung requested changes Dec 23, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Dec 23, 2025

	// TODO: getrandom 0.2 uses the wrong return type for BCrypteGenRandom
	// On Windows, getrandom 0.2 uses the wrong return type for BCrypteGenRandom.

Convert Windows to use check_shim_sig instead of check_shim_sig_lenient #4779

Are you sure you want to change the base?

Convert Windows to use check_shim_sig instead of check_shim_sig_lenient #4779

Conversation

CraftSpider commented Dec 19, 2025

Uh oh!

rustbot commented Dec 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CraftSpider Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CraftSpider Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Dec 19, 2025

Uh oh!

RalfJung left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CraftSpider Dec 19, 2025 •

edited

Loading

CraftSpider Dec 19, 2025 •

edited

Loading

RalfJung left a comment •

edited by rustbot

Loading