RFR: 8274379: Allow process of unsafe access errors in check_special_condition_for_native_trans

[David Holmes]

On Tue, 28 Sep 2021 17:23:29 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

Hi Patricio,

Like Robbin I find it very difficult to actually track the potential changes in behaviour here to determine that they are in fact correct. Especially as it is not clear what the actual rules are supposed to be in the first place.

More broadly I'm concerned about lumping these together as the unsafe_access_error is not an async exception in the same way that thread.stop produces. There is a degree of asynchrony to them but the rules for checking for a pending ThreadDeath from Thread.stop are quite distinct from those related to unsafe_access_error.

I'm also a bit confused about the original check here as I thought that unsafe_access_error can only arise due to execution of JIT'd code, during which the thread is `_thread_in_java` - so I'm not sure how the native-trans check came to be needed in the first place. ??

Thanks,
David

src/hotspot/share/runtime/thread.cpp line 1640:

> 1638:       // We might have blocked in a ThreadBlockInVM wrapper in the call above so make sure we process pending
> 1639:       // suspend requests and object reallocation operations if any since we might be going to Java after this.
> 1640:       SafepointMechanism::process_if_requested_with_exit_check(this, true /* check asyncs */);

I have to wonder why this isn't handled in the TBIVM?

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

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