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

[Patricio Chilano Mateo]

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 David,

Thanks for looking at this.

> 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.
> 
Most of the changes are just cleanup of the API. The only change in behavior is that now in the transition from native back to Java we will throw the InternalError exception if there is a pending unsafe access error. Today we will return to the Java method and throw the exception at some undefined point later on, i.e. whenever we happen to hit the next transition from vm to Java where we will check the async exception condition. So for those unsafe access errors that happened within the context of a native method we'll be throwing an async exception closer to where the unsafe access actually happened. Maybe I can separate the commits into the functional changes and the cleanup? Or otherwise I can just leave the cleanup for another RFE.

> 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.
> 
Right, but today they are already being handled together. They both use the _async_exception_condition field and except for this native to Java special case that I'm removing they are checked and handled at the sample places. If at some point later on we find a place where we should check and handle one but not the other we can always assign them different bits from the suspend_flag. Although I doubt that will happen since this code has been like this forever. Also if we were to completely decouple unsafe access errors from async exceptions and somehow process them synchronously then this change just reduces the amount of code needed to be changed. We just wouldn't set the suspend _flag bit and remove the else condition from check_and_handle_async_exceptions().

> 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. ??
> 
An unsafe access error can also happen when the JT has a state of _thread_in_vm or _thread_in_native while executing methods like Unsafe_CopyMemory0 or Unsafe_CopySwapMemory0. In fact that is actually where we set the unsafe access error condition in test InternalErrorTest.java.

Thanks,
Patricio

Hi Robbin,

> Hi @pchilano,
> 
> Yes this seem like an improvement, so process_if_requested_with_exit_check is the only place were we handle async exceptions. (except JNI) It looks good, but I have a really hard time looking at the diff/code to determine if this behaves differently in any bad ways. I trust you have done the testing needed to be sure.
> 
Thanks for the review! Please check my answer to David regarding maybe separating the commits or just leaving the cleanup to another RFE. 

Thanks,
Patricio

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

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