Replace GetOsPageSize() with minipal_getpagesize() in CoreCLR#127904
Replace GetOsPageSize() with minipal_getpagesize() in CoreCLR#127904pavelsavara wants to merge 2 commits intodotnet:mainfrom
GetOsPageSize() with minipal_getpagesize() in CoreCLR#127904Conversation
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR standardizes CoreCLR’s “OS page size” queries by removing the utilcode GetOsPageSize() wrapper and migrating call sites to the canonical minipal_getpagesize() API (including updating a few page-size-related macros and PAL tests). This helps ensure consistent semantics across platforms (notably WASM’s 16KB page size behavior).
Changes:
- Removed
GetOsPageSize()/GetOsPageSizeUncached()from utilcode and updated all CoreCLR call sites to useminipal_getpagesize(). - Added/propagated
minipal/ospagesize.hinclusion (viautilcode.hand directly in a couple PAL tests). - Cleaned up some page-size-related macro definitions (duplicate define removal + extra parentheses).
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/PrecodeStubsTests.cs | Updates embedded native-code comment to reflect minipal_getpagesize(). |
| src/coreclr/vm/virtualcallstub.cpp | Uses minipal_getpagesize() for reserve/commit alignment and page computations. |
| src/coreclr/vm/threads.h | Replaces page-size macros with minipal_getpagesize() and removes duplicate define. |
| src/coreclr/vm/threads.cpp | Replaces page-size uses in asserts/debug logging/guard-page math with minipal_getpagesize(). |
| src/coreclr/vm/peimagelayout.cpp | Uses minipal_getpagesize() for mapping alignment and validation. |
| src/coreclr/vm/loaderallocator.cpp | Uses minipal_getpagesize() for collectible heap sizing constants. |
| src/coreclr/vm/jitinterface.h | Updates UNIX null-check offset definition to use minipal_getpagesize(). |
| src/coreclr/vm/jitinterface.cpp | Reports osPageSize via minipal_getpagesize() in EE info. |
| src/coreclr/vm/i386/jitinterfacex86.cpp | Updates debug asserts to use minipal_getpagesize(). |
| src/coreclr/vm/hosting.cpp | Updates UEF section page calculations to use minipal_getpagesize(). |
| src/coreclr/vm/frames.cpp | Updates “frame order” relaxation logic to use minipal_getpagesize(). |
| src/coreclr/vm/excep.h | Uses minipal_getpagesize() for NULL_AREA_SIZE on UNIX. |
| src/coreclr/vm/debughelp.cpp | Uses minipal_getpagesize() when walking/touching pages for readability checks. |
| src/coreclr/vm/codeman.h | Updates rounding-to-page macros and comments to use minipal_getpagesize(). |
| src/coreclr/vm/ceemain.cpp | Aligns mini-metadata buffer sizing using minipal_getpagesize(). |
| src/coreclr/vm/appdomain.hpp | Updates loader heap reserve/commit sizing macros to use minipal_getpagesize(). |
| src/coreclr/utilcode/util.cpp | Removes the old GetOsPageSize* implementations. |
| src/coreclr/utilcode/loaderheap.cpp | Uses minipal_getpagesize() for commit sizing alignment. |
| src/coreclr/utilcode/interleavedloaderheap.cpp | Uses minipal_getpagesize() for alignment/assertions. |
| src/coreclr/utilcode/explicitcontrolloaderheap.cpp | Uses minipal_getpagesize() for commit block sizing and alignment. |
| src/coreclr/utilcode/executableallocator.cpp | Uses minipal_getpagesize() when randomizing preferred range start. |
| src/coreclr/utilcode/dacutil.cpp | Uses minipal_getpagesize() for page-boundary-limited reads. |
| src/coreclr/utilcode/clrhost_nodependencies.cpp | Uses minipal_getpagesize() for page-aligned region checks. |
| src/coreclr/pal/tests/palsuite/miscellaneous/IsBadWritePtr/test3/test3.cpp | Includes ospagesize.h and uses minipal_getpagesize() in allocations/checks. |
| src/coreclr/pal/tests/palsuite/miscellaneous/IsBadWritePtr/test2/test2.cpp | Includes ospagesize.h and uses minipal_getpagesize() in allocations/checks. |
| src/coreclr/inc/utilcode.h | Includes minipal/ospagesize.h and removes GetOsPageSize() declaration. |
| src/coreclr/inc/pedecoder.inl | Uses minipal_getpagesize() when temporarily setting decoder size to 2 pages. |
| src/coreclr/inc/loaderheap.h | Updates stub code page sizing logic to use minipal_getpagesize(). |
| src/coreclr/debug/di/shimlocaldatatarget.cpp | Uses minipal_getpagesize() for page-boundary-limited reads. |
| src/coreclr/debug/daccess/enummem.cpp | Uses minipal_getpagesize() for page-chunked memory reporting. |
|
|
||
| PageOne = VirtualAlloc(NULL, | ||
| GetOsPageSize()*4, | ||
| (uint32_t)minipal_getpagesize()*4, |
There was a problem hiding this comment.
VirtualAlloc argument is SIZE_T. This cast should be unnecessary. Can we do a pass to delete the uint32_t casts where it is not necessary or where it is easy to fix the code to avoid it?
(Alternatively, I am wondering whether it would be better for minipal_getpagesize to return uint32_t to avoid these casts.
Fixes #127550
Summary
Remove the
GetOsPageSize()wrapper fromutilcodeand replace all call sites in CoreCLR with the newminipal_getpagesize()API introduced in #127328.Motivation
PR #127328 introduced
minipal_getpagesize()insrc/native/minipal/ospagesize.has the canonical way to query the OS page size across all platforms:4096(no syscall)16384(reduced from the 64KBmemory.growgranularity)getpagesize()result (queried once per process)The old
GetOsPageSize()insrc/coreclr/utilcode/util.cppduplicated this logic with slightly different behavior (usedGetSystemInfo().dwAllocationGranularityon the PAL path, hardcoded0x1000on Windows). Consolidating on the minipal version eliminates the duplication and ensures consistent page-size semantics, particularly for WASM where the correct value is 16KB.Changes
GetOsPageSize()andGetOsPageSizeUncached()definitions fromsrc/coreclr/utilcode/util.cppGetOsPageSize()declaration fromsrc/coreclr/inc/utilcode.h#include <minipal/ospagesize.h>toutilcode.h(covers all VM/utilcode consumers) and to the two PAL test files that don't includeutilcode.hGetOsPageSize()call sites withminipal_getpagesize(), applying(uint32_t)casts where the target storage or comparison type is 32-bitHARD_GUARD_REGION_SIZEbeing defined twice inthreads.h(removed the redundant first definition)SIZEOF_DEFAULT_STACK_GUARANTEEandHARD_GUARD_REGION_SIZEmacro hygiene (wrapped in outer parentheses, removed pointless1 *multiplier)