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

[Jamsheed]

Hi Vladimir,

the runtime calls uses indirect call, and it is not that straight 
forward to compare dst i guess.

will use doing_unsafe_access and error table as you suggested, 
doing_unsafe_access for intrinsic call doesn't require volatile 
semantics in c2 i believe.

all code whose behavior is unpredictable will be removed. like arm 
int/c1, non intrinsic c2 (And other platforms).

Best regards,

Jamsheed

On 11/04/19 5:17 AM, Jamsheed wrote:
> 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
>>>>>