RFR 8191278: MappedByteBuffer bulk access memory failures are not handled gracefully

[Jamsheed]

Hi Vladimir,

On 10/04/19 10:03 PM, Vladimir Kozlov wrote:
> Okay, I see what you did. But it seems incomplete. You did not set 
> continue_pc for some platforms. Why?
for some platform i don't have testing setup, others are not very much 
used in servers(32 bit case).
>
> Also this will trigger the code if we hit segv for normal arraycopy. 
> You may want to lookup caller frame to get address from call 
> instruction and compare it with _unsafe_arraycopy:
>
> if (StubCodeDesc::desc_for(pc)) {
>   frame fr = os::fetch_frame_from_context(uc);
>   address ret_pc = fr.sender_pc();
>   CodeBlob* cb = CodeCache::find_blob_unsafe(ret_pc);
>   CompiledMethod* nm = (cb != NULL) ? cb->as_compiled_method_or_null() 
> : NULL;
>   if (nm != NULL && NativeCall::is_call_before(ret_pc)) {
>     address dest = nativeCall_before(ret_pc)->destination();
>     if (dest == StubRoutines::_unsafe_arraycopy) {
>
> But you need to verify if it works.

this should work i guess.

Best regards,

Jamsheed

>
> Thanks,
> Vladimir
>
> On 4/9/19 8:08 PM, Jamsheed wrote:
>> Hi Vladimir,
>>
>> Thank you for looking at this.
>>
>> On 10/04/19 4:01 AM, Vladimir Kozlov wrote:
>>> Hi Jamsheed,
>>>
>>> Instead of finding PC in stubs we should use something similar to 
>>> GuardUnsafeAccess to set thread's doing_unsafe_access flag when we 
>>> call copy stub for unsafe memory as you suggested first (in bug 
>>> report).
>>>
>>> Interpreter set the flag for Unsafe.CopyMemory0() and 
>>> Unsafe.copySwapMemory0(). C2 has intrinsic only for CopyMemory0():
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/6ad0281a654e/src/hotspot/share/opto/library_call.cpp#l4189 
>>>
>>>
>>> It only use unsafe_arraycopy stab:
>>>
>>> http://hg.openjdk.java.net/jdk/jdk/file/6ad0281a654e/src/hotspot/cpu/x86/stubGenerator_x86_64.cpp#l2434 
>>>
>>>
>>> Setting on it's entry and cleaning on exit 
>>> Thread::_doing_unsafe_access field should be enough. Right?
>>>
>>> Or I am missing something?
>>
>> initially thought of implementing it that way[1], but as it is having 
>> both store and volatile semantics went with this zero overhead solution.
>>
>> also, that doesn't provide me  continuation pc, which is required for 
>> fast exit for bulkaccess  or even for graceful exit in platform like 
>> arm.
>>
>>>
>>> An other thing which bothering me is Harold's comment:
>>>
>>> "Unfortunately, "CompiledMethod* nm" gets set to NULL and so 
>>> handle_unsafe_access() is not executed."
>>>
>>> Where/why nm is NULLed?
>> as we are in BufferBlob/RuntimeBlob(stub frame).
>>>
>>> Actually I think the whole code for "// BugId 4454115" is 
>>> questionable since it replaces any crash (most likely not related to 
>>> unsafe access) in compiled method which has at least one unsafe 
>>> access with exception. May be we should use PcDesc to record unsafe 
>>> instructions and compare with SEGV PC. But it is separate RFE. For 
>>> this one we need to fix only Unsafe.CopyMemory0() C2 inrinsic.
>>
>> Right, Ok.
>>
>> Best regards,
>>
>> Jamsheed
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>> [1]http://cr.openjdk.java.net/~jcm/8191278/webrev.00/src/hotspot/share/opto/library_call.cpp.udiff.html 
>>
>>> On 4/8/19 4:21 AM, Tobias Hartmann wrote:
>>>> Hi Jamsheed,
>>>>
>>>> On 05.04.19 15:11, Jamsheed wrote:
>>>>> On 04/04/19 7:23 PM, Andrew Haley wrote:
>>>>>>> this looks reasonable to me although the code is getting quite 
>>>>>>> complicated to handle this edge case.
>>>>>> Yeah, it really is. Can't we just assume that *any* fault in 
>>>>>> these stubs is
>>>>>> caused via Unsafe, and get rid of bool unsafe_copy_code_range?
>>>>>
>>>>> Thanks for the feedback Tobias, Andrew, removed 
>>>>> unsafe_copy_code_range.
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~jcm/8191278/webrev.04/
>>>>
>>>> I think what Andrew meant is basically to go with webrev.01:
>>>> http://cr.openjdk.java.net/~jcm/8191278/webrev.01/
>>>>
>>>> Right?
>>>>
>>>> Thanks,
>>>> Tobias
>>>>