RFR: 8335362: [Windows] Stack pointer increment in _cont_thaw stub can cause program to terminate with exit code 0xc0000005 [v3]

[Patricio Chilano Mateo]

On Fri, 6 Sep 2024 05:51:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix update in map_stack_shadow_pages
>
> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 289:
> 
>> 287:     address last_touched_page = watermark - StackOverflow::stack_shadow_zone_size();
>> 288:     size_t pages_to_touch = align_up(watermark - new_sp, page_size) / page_size;
>> 289:     while (pages_to_touch--) {
> 
> Suggestion:
> 
>     while (pages_to_touch-- > 0) {

Fixed.

> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 293:
> 
>> 291:       *last_touched_page = 0;
>> 292:     }
>> 293:     thread->stack_overflow_state()->set_shadow_zone_growth_watermark(new_sp);
> 
> I'm not familiar with the details of this stack management code and am unclear about the role of the `shadow_zone_growth_watermark` here. The banging code in `os::map_stack_shadow_pages` doesn't access it.

The shadow zone growth watermark is just an optimization to avoid banging pages that were already touched. It is set to the highest sp (stack growing up) where we banged already (there is a diagram and more explanations in stackOverflow.hpp). So we don't strictly need it but we would incur in unnecessary overhead without it when the size of the frames freezed in the top stackChunk are a couple of pages in size. By checking this reference first we guarantee that almost all the time we won't have to do anything. I added the update of the watermark in os::map_stack_shadow_pages().

> test/jdk/java/lang/Thread/virtual/BigStackChunk.java line 47:
> 
>> 45:         int i6 = i5 + 1;
>> 46:         int i7 = i6 + 1;
>> 47:         long ll = 2*(long)i1;
> 
> Suggestion:
> 
>         long ll = 2 * (long)i1;

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20862#discussion_r1747401722
PR Review Comment: https://git.openjdk.org/jdk/pull/20862#discussion_r1747401934
PR Review Comment: https://git.openjdk.org/jdk/pull/20862#discussion_r1747401862