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

[Jamsheed]

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