RFR: 8325937: runtime/handshake/HandshakeDirectTest.java causes "monitor end should be strictly below the frame pointer" assertion failure on AArch64 [v3]

[David Holmes]

On Thu, 3 Oct 2024 22:51:07 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Add missing StoreLoad fences in handshaking code to match safepoint code.  Thanks to @pchilano for finding this bug.
>> 
>> Tested with tier1-4 and tier8 which has Kitchensink in it.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   How about this fence() and comment in do_self_suspend()?

src/hotspot/share/runtime/handshake.cpp line 706:

> 704:   // Separate all the writes above for other threads reading state
> 705:   // set by this suspended thread.
> 706:   OrderAccess::fence();

IIUC what @robehn is concerned about is stuff that happens before we even take the lock, being reordered with the state change to `_thread_blocked` which happens in `do_thread` before this is called, so this placement is far too late to prevent that reordering.  If a fence is needed then it would be just before (or inside) `set_thread_state`.

Note `set_thread_state` already has release semantics on some platforms (obviously there is disagreement about the need for, or placement of, this). And `set_thread_state_fence`, has the fence in the wrong place for the current problem. We seem to want a `fence_and_set_thread_state`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21295#discussion_r1787009041