RFR: 8298733: Reconsider monitors_on_stack assert [v2]

[Coleen Phillimore]

On Wed, 16 Apr 2025 12:33:08 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> 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.

Am I that predictable? :)  Okay, I see the double code/loop is temporary and one has future usefulness.

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

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