WinApi SYSTEM_INFO dwPageSize always returning 0
#2136
Comments
|
Thanks for the report! Miri's Windows support is a big hack, barely enough to get our own test suite to pass. Windows is effectively at best a Tier 2 target in Miri. (Not that we have a formal target policy.) Basically, I don't have a lot of time to spend on Windows support in Miri myself. But help with the Windows support would certainly be appreciated. :D This is probably caused by some code in this file, though I do not know which function is responsible. Maybe |
|
That would certainly be this code: miri/src/shims/windows/foreign_items.rs Lines 112 to 126 in b96610b If I read it correctly, it fills the entire SYSTEM_INFO structure with zeroes and then sets the CPU count to the actual value. |
|
Yes, that's exactly what it does. We call Looks like we should add the page_size crate as well, then. (But somewhere in this file, not mixed up with the subcrate test.) |
|
Hah, but the use winapi::um::sysinfoapi::{SYSTEM_INFO, LPSYSTEM_INFO};
use winapi::um::sysinfoapi::GetSystemInfo;
#[inline]
pub fn get() -> usize {
unsafe {
let mut info: SYSTEM_INFO = mem::zeroed();
GetSystemInfo(&mut info as LPSYSTEM_INFO);
info.dwPageSize as usize
}
} |
RalfJung
added
A-shims
|
Okay, this should be easy to fix then. :) |
|
I don't see why that'd make it easy, doesn't that mean we couldn't use
|
|
Miri would call page_size as native dependency rather than as interpeted code which means that GetSystemInfo ends up as native system call rather than in the miri shim. |
|
Actually, I can ask more focused questions.
Lines 34 to 38 in b96610b I see that these constants are also used in posix shim to set some variables. Should we also do justice to this FIXME and switch this to the actual underlying page size? |
Sorry, what I meant is call page_size in a test inside Miri to ensure that we are implementing whatever APIs it needs on the platforms we support.
Actually I was thinking Miri would just hard-code 4096 as the pagesize, as it does on Unix.
Right, we may want to do that, but then we should do it on all platforms. So for now I think it should just also use PAGE_SIZE on windows. Basically you would adjust this code to also fill out the page size field. Then if you want, open a separate issue for no longer hard-coding the page size, but making it user-configurable somehow. |
|
Got it. I need guidance on how to test it, though, where would I put the I guess the test would land in |
|
You can add it as a dep in test-cargo-miri/Cargo.toml, and then add a test in test-cargo-miri/tests/test.rs
|
|
There's one thing I don't understand at all. The layout of layout: TyAndLayout {
ty: winapi::um::sysinfoapi::SYSTEM_INFO,
layout: Layout {
fields: Arbitrary {
offsets: [
Size(0 bytes), // 0, u : SYSTEM_INFO_u
Size(4 bytes), // 1, dwPageSize : DWORD
Size(8 bytes), // 2, lpMinimumApplicationAddress : LPVOID
Size(16 bytes), // 3, lpMaximumApplicationAddress : LPVOID
Size(24 bytes), // 4, dwActiveProcessorMask : DWORD_PTR
Size(32 bytes), // 5, dwNumberOfProcessors : DWORD
Size(36 bytes), // 6, dwProcessorType : DWORD
Size(40 bytes), // 7, dwAllocationGranularity : DWORD
Size(44 bytes), // 8, wProcessorLevel : WORD
Size(46 bytes)],// 9, wProcessorRevision : WORD
memory_index: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
},
variants: Single { index: 0 },
abi: Aggregate { sized: true },
largest_niche: None,
align: AbiAndPrefAlign {
abi: Align(8 bytes),
pref: Align(8 bytes)
},
size: Size(48 bytes)
}
}
}So number of processors is the 5th field. But the existing code writes to the 6th: miri/src/shims/windows/foreign_items.rs Lines 122 to 126 in b96610b From this, the page size field must be either the 1st field (as in the layout) or the 2nd field (4 fields before 2ndIf I write: let page_size = this.mplace_field(&system_info, 2)?;
this.write_scalar(Scalar::from_int(PAGE_SIZE, dword_size), &page_size.into())?;then 1stIf I write: let page_size = this.mplace_field(&system_info, 1)?;
this.write_scalar(Scalar::from_int(PAGE_SIZE, dword_size), &page_size.into())?;then I tried digging into this but I found nothing apart from confusion. |
|
FWIW, that existing code is unnecessarily roundabout -- this would be better written as let num_cpus = this.mplace_field(&system_info, 6)?;
this.write_scalar(Scalar::from_u32(NUM_CPUS), &num_cpus.into())?;and changing the type of NUM_CPUS to be Regarding your problem, maybe there is something odd with the type signature used by num_cpus? You can find it here, and If you want you can adjust the code to not use the caller signature at all, and instead hard-code offsets and types. This is the more correct thing to do. (Well, the |
|
I think that is the crux of the issue. I don't know where the layout in the
I think that can be detected though. Maybe the solution would be to check whether the first field is of size 4 or size 2 and offset according to that? In the first case use field 1 for page size and 5 for num cpus, in the latter use fields 2 and 6, respectively? |
|
The layout in foreign_itens comes from the declaration of the call site that we are currently emulating.
Checking field counts sounds rather fragile. I think it is better to ignore the layout and hard-code the offsets.
|
|
The checking field counts approach led to simpler code, but I reimplemented it by explicitly using |
|
I implemented dynamically calculating the offsets of all fields in SYSTEM_INFO, since the concrete values depend on target's pointer size. With that adding values to other fields in that structure should be relatively straightforward. And now it indeed works (🥳). |
Dereference pointers in shims as correct types Currently, shims will dereference pointers as the type written by the user. This can cause false positives, incorrect behavior such as #2136, and even ICEs if a field is not present. This PR fixes this by having shims dereference pointers with types from `std` or `libc` that we can rely on the layout and field names of instead of with whatever the user passed in. Fixes #1123
This is a rather weird issue I reported first in the
page_sizecrate. When running Miri with any of the Tier 1 Windows targets the crate reports page size 0, which is a fatal error for my use, where I want to allocate aligned to the page boundary.This is witnessed most easily by the GitHub Action run. My code panics when the page size is not a power of two, so in this case it crashes with:
The page size query works perfectly well for Unix targets.
Replication
This can be confirmed by referencing
page_sizeand asking for it like this:and running Miri:
resulting in:
Whereas running it without Miri gives the correct result (on my PC it's 4KB):
If it matters, the host OS seems to not affect this, the issue is the same running the command both from Unix and Windows.
The text was updated successfully, but these errors were encountered: