RFR: 8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes

[David Holmes]

On Thu, 29 Jul 2021 20:29:27 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

> A trivial follow-up to:
> 
> JDK-8271251 JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6"
> 
> that adds a stronger sanity check of thread state when polling for safepoint/handshakes.
> 
> This fix was used to test @pchilano's fix for JDK-8271251 in my JDK17 Mach5
> Tier[1-8] runs for JDK-8271251. It has also been tested with Mach5 Tier[1-3]
> for jdk/jdk (JDK18).

Hi Dan,

Not sure about part of this - see below.

Thanks,
David

src/hotspot/share/runtime/safepoint.cpp line 708:

> 706: 
> 707:   JavaThreadState state = thread->thread_state();
> 708:   assert(SafepointSynchronize::is_a_block_safe_state(state), "Illegal threadstate encountered: %d", state);

Nit: you shouldn't need to say SafepointSynchronize:: here.

src/hotspot/share/runtime/safepoint.cpp line 716:

> 714:   // So we can miss this poll, but stop at next.
> 715: 
> 716:   // Load dependent store, it must not pass loading of safepoint_id.

Existing: I struggle to understand what this comment means - we are storing the value of safepoint_id so I don't see how the loading of safepoint_id can be reordered. ???

src/hotspot/share/runtime/safepointMechanism.cpp line 122:

> 120:   do {
> 121:     JavaThreadState state = thread->thread_state();
> 122:     guarantee(SafepointSynchronize::is_a_block_safe_state(state), "Illegal threadstate encountered: %d", state);

Shouldn't this be outside the loop? Though we are doubling up given the assert in block().
Do we really want guarantee rather than assert? Doesn't a failure here indicate an internal programming error?

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

PR: https://git.openjdk.java.net/jdk/pull/4936