On Sun, 23 Aug 2026 05:22:07 GMT, Ashay Rane <[email protected]> wrote:

>> Prior to this patch, the `StackYellowPages` and `StackRedPages`
>> influenced the total reserved stack memory, but they didn't change the
>> minimum stack size specified as required by HotSpot to Windows.  This
>> patch adds the call to `SetThreadStackGuarantee()` to set the minimum
>> stack size.
>> 
>> The key change here is that the argument to `SetThreadStackGuarantee()`
>> is set to one less than the total number of yellow pages.  The one less
>> page is because Windows uses a guard page (access to which tells Windows
>> to commit more stack pages); in the terminal case, we want the guard
>> page to coincide with the top-most yellow stack page, thus leaving all
>> except one yellow page available as the stack size.
>> 
>> Importantly, however, we do not use the red page count as the argument
>> to `SetThreadStackGurantee()`, since the red stack pages are unavailable
>> for handling recoverable overflows.  The red page count impacts the
>> _total reserved_ stack size, just not the _minimum_ stack size.  Still,
>> forcing the red stack page count to be included into the computation of
>> the argument to `SetThreadStackGurantee()` causes HotSpot to fail with
>> the assertion `assert(!in_vm) failed: Undersized StackShadowPages`,
>> since Windows is unable to commit more stack pages due to the minimum
>> stack size now being larger than just the yellow page count minus one.
>> 
>> The accompanying test passes zero to `SetThreadStackGuarantee()` to
>> probe the current minimum stack size, which we then compare against the
>> expected size based on the yellow page count.  The same test fails
>> without this patch on both Windows/x64 and on Windows/ARM64.
>> 
>> Validated this patch by running through all tier 1, 2, and 3 HotSpot
>> jtreg tests on Windows/x64 and Windows/ARM64 in FastDebug config.
>> 
>> ---------
>> - [x] I confirm that I make this contribution in accordance with the 
>> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).
>
> Ashay Rane has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Mark stack pages using `PAGE_NOACCESS` instead of `PAGE_GUARD`

The test/jdk/java/lang/ScopedValue/StressStackOverflow.java test consistently 
fails on Windows/x64 in CI (although I can't reproduce it locally).  
Interestingly, the same test passes on Windows/ARM64, and I wonder if it's 
because of the difference in `DEFAULT_STACK_SHADOW_PAGES`, which has the 
following comment in the initialization for x64:

```c++
// Java_java_net_SocketOutputStream_socketWrite0() uses a 64k buffer on the
// stack if compiled for unix. To pass stack overflow tests we need 20 shadow 
pages.
#define DEFAULT_STACK_SHADOW_PAGES (NOT_WIN64(20) WIN64_ONLY(8) DEBUG_ONLY(+4))
// For those clients that do not use write socket, we allow
// the min range value to be below that of the default
#define MIN_STACK_SHADOW_PAGES (NOT_WIN64(10) WIN64_ONLY(8) DEBUG_ONLY(+4))


whereas for AArch64, it is initialized as:

```c++
// Java_java_net_SocketOutputStream_socketWrite0() uses a 64k buffer on the
// stack if compiled for unix and LP64. To pass stack overflow tests we need
// 20 shadow pages.
#define DEFAULT_STACK_SHADOW_PAGES (20 DEBUG_ONLY(+5))
#define MIN_STACK_SHADOW_PAGES DEFAULT_STACK_SHADOW_PAGES


I'm not familiar with how these initial values are decided, but it sounds like 
it may have been through trial and error.  Is it fair to make the 
initialization on ~Windows~ x64 to be uniform for all OSes like the following?

```c++
// Java_java_net_SocketOutputStream_socketWrite0() uses a 64k buffer on the
// stack if compiled for unix. To pass stack overflow tests we need 20 shadow 
pages.
#define DEFAULT_STACK_SHADOW_PAGES (20 DEBUG_ONLY(+4))
// For those clients that do not use write socket, we allow
// the min range value to be below that of the default
#define MIN_STACK_SHADOW_PAGES (10 DEBUG_ONLY(+4))

-------------

PR Comment: https://git.openjdk.org/jdk/pull/32365#issuecomment-5386410319

Reply via email to