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

[David Holmes]

HI Mikael,

On 11/04/2016 10:50 AM, David Holmes wrote:
> Hi Mikael,
>
> 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/
>>
>
> I'm not at all sure this is going in the right direction and I certainly
> need some time to look at it in detail. I'm unclear what the timeliness
> issue actually is, as converting errors to exceptions only makes sense
> when entering/returning-to Java code.

Can you elaborate on the timeliness problem please. The only way I could 
see this being an issue is if there is native loop using the unsafe 
access code, which also checks for pending exceptions. Otherwise you 
should see the exception thrown as soon as Java code is hit.

I don't like using the preallocated InternalError instance precisely 
because we lose the stack information - something I would have thought 
to be the most important part!

> My initial reaction is to not entangle these two completely different
> forms of exceptional conditions. The unsafe access error is not really
> asynchronous at all - it is a synchronous condition! - but conversion to
> an exception is deferred which makes it appears as an asynchronous
> exception at the Java level.

Well there are three somewhat tangled "async" checks (external suspend, 
async-exceptions, unsafe access error) so always checking for unsafe 
access errors may not be an issue in itself - it is only the allocating 
of the InternalError that prohibits doing this presently. But as I said 
I think the inline allocation with a stacktrace is the most desired 
thing to have.

Thanks,
David

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