RFR(S): 8153890: Handle unsafe access error as an asynchronous exception

[David Holmes]

The light is starting to shine ... :)

On 12/04/2016 10:35 AM, David Holmes wrote:
> Looking at the test ...
>
> On 9/04/2016 8:27 AM, Mikael Vidstedt wrote:
>>
>> Please review:
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8153890
>> Webrev:
>> http://cr.openjdk.java.net/~mikael/webrevs/8153890/webrev.01/hotspot/webrev/
>>
>>
>>
>> * Note: this is patch 1 in a set of 3 all aiming to clean up and unify
>> the unsafe memory getters/setters, along with the handling of unsafe
>> access errors. The other two issues are:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8153892 - Handle unsafe access
>> error directly in signal handler instead of going through a stub
>> https://bugs.openjdk.java.net/browse/JDK-8150921 - Update Unsafe
>> getters/setters to use double-register variants
>>
>>
>> * Summary (copied from the bug description)
>>
>> In certain cases, such as accessing a region of a memory mapped file
>> which has been truncated on unix-style operating systems, a SIGBUS
>> signal will be raised and the VM will process it in the signal handler.
>> Currently, as part of the handling a flag is set on the thread
>> indicating that an error has occurred:
>>
>> _special_runtime_exit_condition = _async_unsafe_access_error;
>>
>> Eventually the VM will check the flag and throw an InternalError.
>> However, the flag is not always checked promptly, which means that the
>> time between the error and the time when the InternalError is thrown can
>> be very long.
>>
>> In parallel with this, there is functionality in the VM to set pending
>> asynchronous exceptions on threads. These asynchronous exceptions are
>> checked/thrown more often.
>>
>> Instead of having two mechanisms for handling these types of
>> asynchronous events the unsafe access errors should be converted to use
>> the normal asynchronous exception mechanism.
>>
>> One of the reasons why this isn't already the case is that when the
>> SIGBUS signal is raised, the thread may not be in a place where it can
>> allocate an exception - there may not be a safepoint on the instruction
>> performing the memory access. The solution to this is to preallocate the
>> InternalError exception at VM start up, and have the handler code simply
>> use that exception instead. The only downside of this is that the
>> exception will not have a stack trace. The flip side of that is that
>> because of the unpredictability in when the async unsafe access error is
>> actually thrown today it is likely that the stack trace in the exception
>> doesn't actually reflect the place where the error actually occurred
>> anyway.
>>
>> * Testing
>>
>> In addition to the JPRT smoke tests a new test has been developed for
>> testing unsafe access errors, in particular accessing data in the
>> truncated portion of a memory mapped file using the Unsafe primitives.
>> The test can be found here:
>>
>> http://cr.openjdk.java.net/~mikael/webrevs/8150921/MappedTruncated.java
>
> Here:
>
>         } else {
>              r.run();
>
>              // The operation may have raised a pending async
>              // exception, make sure it's actually thrown
>              forcePendingAsyncExceptionProcessing();
>          }
>
> forcePendingAsyncExceptionProcessing should not be needed. There is no
> way we should be able to return to Java without the pending exception
> being thrown from r.run()! If we can then there is a serious bug somewhere.

Ah! So now I think I see what the real bug is - it isn't timeliness it 
is a correctness issue. It should be impossible to return from native to 
Java without converting a pending "async error" into an actual 
exception. But the native call stub calls 
JavaThread::check_special_condition_for_native_trans, which in turn calls:

  thread->check_and_handle_async_exceptions(false);

which skips the unsafe-access-error check. And that is wrong if the 
Unsafe methods are called as normal native methods. But we can't do the 
unsafe-access-error check while we are in a "trans" state if it will 
allocate.

The existing code seems to assume that unsafe-access will not occur via 
the normal native call path. Exactly how are these unsafe accesses 
performed?

David
-----

> Thanks,
> David
>
>> The plan is to push it together with the final patch (JDK-8150921). Note
>> though that the test fails unless the other patches mentioned above are
>> also applied.
>>
>> I am also looking for to run additional testing on this before pushing,
>> but wanted to get some early feedback.
>>
>> Cheers,
>> Mikael
>>