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

[Daniel D.Daugherty]

On Thu, 29 Jul 2021 22:04:56 GMT, David Holmes <dholmes 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).
>
> 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.

Agreed. Will fix it.

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

I'll have to check to see who added that comment.

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

I think the thread's state can change during this loop so having the check in the
loop will catch if it changes to something bad.

The assert() in `block()` is to catch any future new callers to `block()` that aren't
calling from the right thread_state.

Yes, I specifically wanted a `guarantee()` here to catch this condition in 'release' bits.
The original internal programming bug was racy and I want to make sure we have the
best chance to catch any future racy uses.

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

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