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

[David Holmes]

On Tue, 5 Oct 2021 19:57:32 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Hi,
>> 
>> 
>> Please review the following patch to allow processing of unsafe access errors in check_special_condition_for_native_trans().
>> 
>> Today we special case unsafe access errors from other async exceptions in that we don't process them when transitioning from native back to Java. Code comments in check_special_condition_for_native_trans() mention that unsafe access errors should not be handled because that may block while creating the exception. But that should not be an issue since we can always make sure we call process_if_requested_with_exit_check() after the call to throw_unsafe_access_internal_error() to process any pending operations not already handled in a ThreadBlockInVM wrapper (today that only means suspend requests and object reallocation operations).
>> 
>> By removing this special treatment for unsafe access errors we can also simplify the async exception support API. For instance we can remove _async_exception_condition and simplify some of the supporting methods. I also removed the _thread_in_native case from the switch statement in check_and_handle_async_exceptions() since we never call that method in that state.
>> 
>> Testing by running tiers1-6 in mach5. 
>> 
>> Thanks,
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - async API cleanup
>  - general cleanup
>  - allow process of unsafe access errors in check_special_condition_for_native_trans
>  - revert to separate commits

Hi Patricio,

Thanks for the separate commits. I hope github makes it clear where comments are being directed.

This is a comment on the initial functional change, which generally seems okay. One detauiled comment below.

Thanks,
David

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 :) ).

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

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