RFR: 8298733: Reconsider monitors_on_stack assert

[Fredrik Bredberg]

On Tue, 15 Apr 2025 20:01:11 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> This PR aims to revert this [8298371: monitors_on_stack extracts unprocessed oops](https://github.com/openjdk/jdk/pull/11582) fix at the same time as we retain the ability to assert that we don't have any monitors on the stack when legacy locking is used.
>> 
>> It does so by moving an assert that ensures that we don't have any monitors on the stack to a place where all stack frames should have been processed. I.e. it's safe to check the frame for any monitor.
>> 
>> A new function called assert_frames_in_continuation_are_safe() asserts that the frames has been processed (i.e. it  safe to revert the [8298371](https://github.com/openjdk/jdk/pull/11582) fix). By keeping that assertion functionality separate from monitors_on_stack() we will be able to keep that assertion after we have removed legacy locking.
>> 
>> In order to recreate the original [problem](https://github.com/openjdk/jdk/pull/11582) and verifying that this PR works used, I used:
>> `-XX:LockingMode=1 -XX:+UseZGC -XX:+ZVerifyOops -XX:ZCollectionIntervalMajor=0.001 Fuzz.java
>> `
>> 
>> Also passes tier1-7.
>
> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 594:
> 
>> 592:                   RegisterMap::WalkContinuation::skip);
>> 593:   map.set_include_argument_oops(false);
>> 594:   for (frame f = thread->last_frame(); Continuation::is_frame_in_continuation(ce, f); f = f.sender(&map)) {
> 
> We walk the stack twice - once to assert_is_frame_safe() for the stack watermark and then this set of calls about owning locks.  Why not just add if (watermark != nullptr) watermark->assert_is_frame_safe(f); here.  Why is the same thing twice - ie getting the RegisterMap?
> I thought the problem with monitors_on_stack was that the watermark was not safe for older (higher) frames?  Is it that at this call location, the stack watermark has been processed?

I knew you where going to say that... :)
So, my original code looked exactly like what you are asking for (e.g. called `assert_is_frame_safe(f)` in the loop of `monitors_on_stack()`. Then I had a chat with the GC guys and was convinced to move it into a separate `assert_frames_in_continuation_are_safe()`. Yes it will walk the stack twice, but it's only in debug mode, so not  a big deal. It's also a preparation for keeping `assert_frames_in_continuation_are_safe()` in `unwind_frames()` when we remove legacy locking.

You are right, by changing the location of the `assert()` that includes the call to `monitors_on_stack()` to just after `flush_stack_processing()` in `unwind_frames()` we ensure that all frames has been processed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24655#discussion_r2046828080