RFR: 8274379: Allow process of unsafe access errors in check_special_condition_for_native_trans [v3]

[Patricio Chilano Mateo]

On Wed, 6 Oct 2021 00:51:18 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   address David comments
>
> src/hotspot/share/runtime/thread.cpp line 1642:
> 
>> 1640:       // suspend requests and object reallocation operations if any since we might be going to Java after this.
>> 1641:       SafepointMechanism::process_if_requested_with_exit_check(this, true /* check asyncs */);
>> 1642:       break;
> 
> Other than now including the final assertion to be executed, I don't see why we need to change from "return" to "break". That final assertion seems unnecessary as it basically checks that Exceptions::throw... actually threw. I'd be tempted to just delete the final assertion and keep the return statements. But not a major concern either way.

Fixed.

> src/hotspot/share/runtime/thread.cpp line 1850:
> 
>> 1848:   assert(!thread->has_last_Java_frame() || thread->frame_anchor()->walkable(), "Unwalkable stack in native->Java transition");
>> 1849: 
>> 1850:   thread->set_thread_state(_thread_in_vm);
> 
> It seems odd to change the thread-state explicitly here when this logic is being processed in the middle of a transition from native->java. So what we end up doing is native->native_trans->in_vm->in_java (and possibly some additional changes from in_vm to blocked and back). I understand why we need to be "in vm" before calling process_if_requested... but it makes the thread state transition story rather messy (just imagine trying to draw a state machine model for this :) ).

Once we call JavaThread::check_special_condition_for_native_trans() we are calling into the vm so to me it actually makes more sense to set the state to _thread_in_vm there. Specially given that this is not some leaf method, but we actually process safepoint/handshakes/stackwatermarks and all other operations we check for in handle_special_runtime_exit_condition(). 
Staying in native_trans also doesn't simplify the state machine in my opinion and actually makes it more complex because inside process_if_requested_with_exit_check() we have to keep track of all these states different than _thread_in_vm: switch case with valid states for safepoint code, transition wrapper for state bookkeeping in handshake code (so we still have the native_trans->vm change), and having to manually switch to _thread_blocked and back because we cannot use ThreadBlockInVM in wait_for_object_deoptimization().

Note: As a future change I think we could just avoid the native_trans state altogether and just poll in a state of _thread_in_vm, since the only purpose of it is to poll in an unsafe state. Or we could even set the state to _thread_in_Java already, so that when there is nothing to process there are no extra changes of state needed, and when there are pending operations we can use a ThreadInVMfromJava wrapper here.

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

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