RFR 8191278: MappedByteBuffer bulk access memory failures are not handled gracefully
Jamsheed
jamsheed.c.m at oracle.com
Mon Jun 3 18:42:47 UTC 2019
Hi Vladimir, Dan,
added two cases in Assembler::locate_operand as movdqa was
unrecognized. update webrev in place
+ case 0x6F: // movdq
+ case 0x7F: // movdq
full diff : http://cr.openjdk.java.net/~jcm/8191278/webrev.06/
http://cr.openjdk.java.net/~jcm/8191278/webrev.05/ + (incremental patch)
http://cr.openjdk.java.net/~jcm/8191278/webrev.06_02/
Best regards,
Jamsheed
On 31/05/19 10:34 PM, Jamsheed wrote:
> Hi Vladimir, Dan,
>
> On 15/05/19 2:24 AM, Vladimir Kozlov wrote:
>> 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.
> It would be really good if it can be done in runtime, i tried this
> approach as it seemed far more easier.
>
> aarch64 port cpp implementation actually crashes for Unsafe_CopyMemory.
>
>>
>> My main concern is new swap code complicates reliable arraycopy code
>> for very corner case. And you implemented it only for x64 anyway.
>
> copySwap cpp code may work for all cases (depends on compiler again) ,
> so i am ok with removing the changes.
>
> but today when i was testing the removed code[1], found another issue.
> some x86 instructions generated by cpp copyswap code is not
> recognized by our assembler and it crashes in signalHandler
> Assembler::locate_next_instruction(pc). i will investigate and file a
> bug.
>
> Best regards,
>
> Jamsheed
>
> [1]
>
> full diff : http://cr.openjdk.java.net/~jcm/8191278/webrev.06/
>
> http://cr.openjdk.java.net/~jcm/8191278/webrev.05/ + (incremental
> patch) http://cr.openjdk.java.net/~jcm/8191278/webrev.06_02/
>
>>
>> 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-dev
mailing list