On Fri, 6 Nov 2026 22:26:54 GMT, Dean Long <[email protected]> wrote:
>> Ashay Rane has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address PR comments
>>
>> 1. Mark TEB pointer as `volatile` so that read accesses of `StackLimit`
>> are not optimized away. Also mark it as constant since the function
>> does not modify the TEB.
>>
>> 2. Use the return value of `grow_windows_stack_to()` to decide whether
>> to continue program execution or to show the unrecoverable handler
>> error.
>>
>> 3. Attach JBS issue identifier to the test.
>>
>> 4. Allow access to native methods in the test. Also set the stack size
>> to 1MB, so that too small a stack size (e.g. 256KB) does not cause
>> the JVM to crash when running the test.
>>
>> 5. Update miscellaneous comments.
>
> src/hotspot/os/windows/os_windows.cpp line 2696:
>
>> 2694: // and installs a new moving guard. Re-read StackLimit after every
>> touch
>> 2695: // because Windows may grow the stack by more than one page at a
>> time.
>> 2696: while (target < (address)tib->StackLimit) {
>
> So this depends on StackLimit changing, otherwise we get into an infitnite
> loop? Is the compiler guaranteed to treat StackLimit as volatile? Maybe we
> should force it to volatile like we do for "probe", or use an atomic read.
Thanks, good catch. I've updated `tib` to now be `volatile` (in addition to
being `const` since the function doesn't modify the TEB).
> src/hotspot/os/windows/os_windows.cpp line 2704:
>
>> 2702: *probe = 0;
>> 2703:
>> 2704: if ((address)tib->StackLimit >= old_limit) {
>
> Is StackLimit > old_limit possible? Maybe we could asset that it never
> happens.
It shouldn't happen, but instead of an assert, I incorporated Vladimir's
suggestion to use the return value to run the unrecoverable overflow handler,
so that we can catch the unexpected condition in both debug as well as release
builds. Let me know if you'd prefer a different approach.
> test/hotspot/jtreg/runtime/StackGuardPages/libStackLimit.cpp line 31:
>
>> 29:
>> 30: // Compute an address that's comfortably below the existing
>> `StackLimit`.
>> 31: const size_t offset = 256 * 1024;
>
> Codex thinks this could cause a crash if someone runs the test witn -Xss256k.
Thanks for running this through Codex. Yes, indeed, the test crashes when the
stack size is 256KB (as is expected, since the memory access now lands outside
the usable stack, so the access violation exception handler doesn't intercept
it to grow the stack). I've set the stack size for this test to be 1MB using
"-Xss1m".
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507660639
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507657695
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507666160
