RFR: 8358821: patch_verified_entry causes problems, use nmethod entry barriers instead [v2]

[Martin Doerr]

On Thu, 12 Jun 2025 02:01:47 GMT, Dean Long <dlong at openjdk.org> wrote:

>> This PR removes patching of the verified entry point and related code, and replaces it by refactoring the existing nmethod entry barrier.
>> 
>> We used to patch the verified entry point to make sure it was not_entrant.  The patched entry point then redirected to SharedRuntime::handle_wrong_method(), either directly with a jump to a stub, or indirectly with an illegal instruction and the help of the signal handler.  The not_entrant state is a final state, so once an nmethod becomes not_entrant, it stays not_entrant.  We can do the same thing with a permanently armed nmethod entry barrier.
>> 
>> The solution I went with reserves one bit of the entry barrier guard value.  This bit must remain set, so I call it a "sticky" bit.  Setting the guard value now is effectively like setting a bitfield, so I needed to add a lock around it.  The alternative would be to change the platform-specific code to do compare-and-swap.
>> 
>> For the lock, I introduced a new NMethodEntryBarrier_lock, whose only purpose is to make the update to the guard value atomic.  For ZGC, I decided to use the existing per-nmethod lock ZNMethod::lock_for_nmethod().  I suspect we could do the same for Shenandoah, if needed for performance.
>> 
>> This change also makes it a bit clearer that  the nmethod entry barrier effectively has two levels.  Level 0 is the outer level or layer controlled by BarrierSetNMethod::nmethod_stub_entry_barrier(), and the inner layer controlled by BarrierSetNMethod::nmethod_entry_barrier().  This could be generalized if we decide we need more flavors of entry barriers.  The inner barrier is mostly ignorant of the fact that the outer guard is multiplexing for both levels.
>
> Dean Long has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - ... and stale code
>  - removed stale comment

I appreciate this PR. I like using the nmethod entry barrier and getting rid of not-entrant patching.
I have some change requests. In addition, we can get rid of much more code. I've put my proposal in a Commit: https://github.com/TheRealMDoerr/jdk/commit/4aed569dc353c254a2f4de2d387208a0a1323990
Please take a look!

src/hotspot/share/gc/shared/barrierSetNMethod.cpp line 246:

> 244:   ConditionalMutexLocker ml(NMethodEntryBarrier_lock, !NMethodEntryBarrier_lock->owned_by_self(), Mutex::_no_safepoint_check_flag);
> 245:   int value = guard_value(nm) | not_entrant;
> 246:   set_guard_value(nm, value);

Same here.

src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp line 153:

> 151:         // Code cache unloading needs to know about on-stack nmethods. Arm the nmethods to get
> 152:         // mark_as_maybe_on_stack() callbacks when they are used again.
> 153:         _bs->arm(nm);

This breaks the Shenandoah build. `arm` is private.

src/hotspot/share/gc/z/zBarrierSetNMethod.cpp line 116:

> 114:     value |= not_entrant;
> 115:   }
> 116:   set_guard_value(nm, value);

We can avoid redundant patching since we have the lock: Only update if the value is not already there. This happens quite often.

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

Changes requested by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25764#pullrequestreview-2920990811
PR Review Comment: https://git.openjdk.org/jdk/pull/25764#discussion_r2142655962
PR Review Comment: https://git.openjdk.org/jdk/pull/25764#discussion_r2142641974
PR Review Comment: https://git.openjdk.org/jdk/pull/25764#discussion_r2142654627