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

Vladimir Kozlov vladimir.kozlov at oracle.com
Tue May 14 20:54:41 UTC 2019


Sorry, I missed this. Please "ping" if you do not get responses.

It become even more complicated :( but I understand why you are doing this way. You did great job.

I am thinking about separating the fix for arraycopy stubs fix and adding graceful exit for Unsafe_CopyMemory and 
Unsafe_CopySwapMemory.

My main concern is new swap code complicates reliable arraycopy code for very corner case. And you implemented it only 
for x64 anyway.

I would like to hear what Runtime group think.

On 4/30/19 8:17 PM, Jamsheed wrote:
> Hi Vladimir,
> 
> Thank you for all the feedback.
> 
> please find the revised webrev.
> 
> http://cr.openjdk.java.net/~jcm/8191278/webrev.05/
> 
> what changed
> 
> 1) Unsafe Copy Memory regions, error exits are captured using UnsafeCopyMemoryMark, UnsafeCopyMemory.

This is good.

> 
> 2) all Unsafe copy (intrinsic or native) ,uses array copy stub.

Right, otherwise we would have to duplicate logic in Copy:: platform specific C++ methods. But may be it is fine to do 
in C++ in this case. Or not do that at all as other platforms.

> 
> 3) Unsafe copyswap x64 implementation extends arraycopy stub and used as reliable exit(fast exit too)*. it is not 
> implemented for other platforms.

As I said I have concern about this.

Thanks,
Vladimir

> 
> *copySwap was tested using copySwap test in jdk dir(with different buffer sizes)
> 
> newly added test tested on linux(aarch64,arm32,x86(32/64)) and platforms in mach5. + mach(1-5)
> 
> ppc test is not done.
> 
> Best regards,
> 
> Jamsheed
> 
> 
> On 12/04/19 9:14 PM, Vladimir Kozlov wrote:
>> Hi Jamsheed
>>
>> I think new methods and the table don't need to be part of StubRoutines class. Your new class is visible in all places 
>> too. You can move methods and table into new class. Then you don't need long names and new code will be in one place.
>>
>> Vladimir
>>
>> On 4/11/19 11:00 PM, Jamsheed wrote:
>>> Hi Vladimir,
>>>
>>> On 12/04/19 12:20 AM, Vladimir Kozlov wrote:
>>>> On 4/11/19 10:25 AM, Jamsheed wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> the runtime calls uses indirect call, and it is not that straight forward to compare dst i guess.
>>>>
>>>> Okay. Yes, we may load dest into register since it is considered far call (outside CodeCache).
>>>> But at least you can find nmethod. So we can do nm->has_unsafe_access() check.
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Yes, we don't need fragile frame walking if we use doing_unsafe_access.
>>>>
>>>> There is MemBarCPUOrder already in inline_unsafe_copyMemory() which will prevent store instruction moves in 
>>>> generated code. But it does not prevent CPU (Aarch64?) to schedule store in different place.
>>>>
>>>> On other hand we need to read it in Signal handle. I would assume all stores should be flushed when we hit SEGV.
>>> yes
>>>>
>>>> I thought about avoiding your error table. But you really need continuation PC for graceful return.
>>>> I was thinking to have a new separate stub to restore registers and pop frame. But return code in stubs varies 
>>>> unfortunately. So we need a table.
>>>>
>>>> One complain about table is its name too long. And it should be unsafe_copymemory to hint intrinsic. Can it be 
>>>> unsafe_copymemory_error and UnsafeCopyMemoryError class.
>>>> StubRoutines:: is_unsafe_copymemory() and next_pc_for_unsafe_copymemory_error()
>>> yes
>>>>
>>>> I did not get why you providing next PC only for 64 bit VM.
>>>
>>> next_pc is calculated for all case(both 32 bit and 64 bit). this should work for c2-intrisics at-least except for arm.
>>>
>>> fast exit is implemented only for x64, as of now.
>>>
>>>>
>>>>>
>>>>> all code whose behavior is unpredictable will be removed. like arm int/c1, non intrinsic c2 (And other platforms).
>>>>
>>>> Okay.
>>>
>>> so i am planning to remove graceful exit for all unpredictable cases. so old behavior will be seen if there is an 
>>> exit at startup(SIGBUS crash).
>>>
>>> and steady state use will be mostly c2 intrinsic and will have graceful exit.
>>>
>>> Best regards,
>>>
>>> Jamsheed
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> 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
>>>>>>>>>>


More information about the hotspot-runtime-dev mailing list