On Fri, 6 Nov 2026 23:43:58 GMT, Vladimir Kozlov <[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 2787:
>
>> 2785: // If it's a legal stack address map the entire region in
>> 2786: if (thread->is_in_usable_stack(addr)) {
>> 2787: addr = (address)((uintptr_t)addr &
>
> Update the comment.
Updated in the most recent commit.
> src/hotspot/os/windows/os_windows.cpp line 2810:
>
>> 2808: // If it's a legal stack address map the entire region in
>> 2809: if (thread->is_in_usable_stack(addr)) {
>> 2810: grow_windows_stack_to(align_down(addr, os::vm_page_size()));
>
> You don't check result of `grow_windows_stack_to`.
> Should we report error and exit by fall through to VMError below if it
> returns false?:
>
> if (thread->is_in_usable_stack(addr) &&
> grow_windows_stack_to(align_down(addr, os::vm_page_size()))) {
> return EXCEPTION_CONTINUE_EXECUTION;
> }
Yeah, I was debating whether to say:
```c++
return grow_windows_stack_to(align_down(addr, os::vm_page_size()))
? EXCEPTION_CONTINUE_EXECUTION
: EXCEPTION_CONTINUE_SEARCH;
But I like the idea of a hard failure in the event of the unexpected condition.
I've updated the code to incorporate your suggestion. Thanks!
> test/hotspot/jtreg/runtime/StackGuardPages/TestWindowsStackLimit.java line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @summary Verifies that on Windows, HotSpot's access-violation
>> recovery for
>
> Add @bug 8394484
Thanks, sorry to have missed that. Updated in the most recent commit.
> test/hotspot/jtreg/runtime/StackGuardPages/TestWindowsStackLimit.java line 31:
>
>> 29: * @requires os.family == "windows"
>> 30: * @library /test/lib
>> 31: * @run main/othervm/native TestWindowsStackLimit
>
> Add `--enable-native-access=ALL-UNNAMED`
Thanks, fixed! I gather that this flag ensures that native methods are
accessible, but I'm not quite sure why it's needed, since the test works
without the flag as well. Is it so that if the default value for some flags
were to change, then this flag would ensure that this test continues to work?
> test/hotspot/jtreg/runtime/StackGuardPages/TestWindowsStackLimit.java line 48:
>
>> 46:
>> 47: // `Linker.Option.critical()` keeps the calling thread's state as
>> 48: // `_thread_in_java` for the whole native call, which is a
>> prerequisite
>
> `_thread_in_Java`
Fixed the typo.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507678229
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507683015
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507667292
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507676275
PR Review Comment: https://git.openjdk.org/jdk/pull/33729#discussion_r4507677506
