RFR: 8272526: Cleanup ThreadStateTransition class [v2]

[Patricio Chilano Mateo]

On Wed, 18 Aug 2021 01:35:05 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> We just need to transition to an unsafe state before polling to avoid the issue of a safepoint/handshake starting right after we checked but before we transition to the unsafe state. Since we are in the vm we can use _thread_in_vm instead which is already unsafe. We still use the _thread_in_native_trans state on the native wrappers when transitioning out of native back to Java, but I will try to address that in another RFE.
>
> I kind of liked the trans states as indicators of unsafe states as it made the logic consistent and simple:
> - advance to current state + 1 (trans state)
> - check for safepoint
> - advance to final state
> 
> and you didn't have to think too much about what the initial state was.
> 
> But I can see that logically any unsafe state works.

Right, but whenever we are in safepoint/handshake code we are already in vm, that's why I actually think directly setting _thread_in_vm here makes more sense than having additional states just to make the poll. In fact using transitional states(along with the code in handle_polling_page_exception) forces us to need yet another transition wrapper for handshakes(ThreadInVMForHandshake) just to switch to vm and back. 
Reading now the description of transition states in globalDefinitions.hpp I see they were more useful back then with the old safepoint logic:

"These extra states makes it possible for the safepoint code to handle certain thread_states without having to suspend the thread - making the safepoint code faster."

In SafepointSynchronize::block() there was a faster path for _thread_in_native_trans and _thread_blocked_trans (https://github.com/openjdk/jdk/blame/66aa45649ad36ed41c0241ded767d465248eda3c/src/hotspot/share/runtime/safepoint.cpp#L816).

>> I see, maybe I should instead change the error message for the _thread_in_vm case to be "a fault occurred in a recent unsafe memory access operation"?
>
> IIRC it was only the _thread_in_Java case that was special because that was where JIT'd code could have a series of unsafe memory accesses, and the exception won't be thrown until we hit a specific async exception check point.

So today we always reach the _async_unsafe_access_error case of check_and_handle_async_exceptions() with a state of _thread_in_Java, except when called from jni_check_async_exceptions() where we are in vm, so I don't know why the message refers exclusively to compiled code. I run test InternalErrorTest.java in baseline with just the interpreter("main/othervm -Xint -Xbootclasspath/a:.  -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI InternalErrorTest") and still passes. Here is the code that sets the pending unsafe access error when the thread receives SIGBUS while in Java: https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_x86/os_linux_x86.cpp#L249

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

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