Convert Windows to use check_shim_sig instead of check_shim_sig_lenient #4779
Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
| "*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::") => |
There was a problem hiding this comment.
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.
| 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 problem hiding this comment.
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?
| } else { | ||
| CanonAbi::C | ||
| }; | ||
| // TODO: Use this in shim-sig instead of 'system'? Or is that good enough for ExternAbi? |
There was a problem hiding this comment.
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.
| 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), |
There was a problem hiding this comment.
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 betweenisizeand*mut _in the past, and many Windows shims actually treat it as its own special thing.HLOCALandHMODULE: Aliases forHANDLE, but with the implication they represent only certain types of handle.FARPROC: ActuallyOption<extern "C" fn()>. Much easier to load the resolved layout of that, instead of trying to addOptionandfnspecial-cased toshim_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.
There was a problem hiding this comment.
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).
| 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 _), |
There was a problem hiding this comment.
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.
CraftSpider
force-pushed
the
windows-shim-sig
branch
from
4824149 to
9cebf4e
Compare
|
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. |
3 participants
No description provided.