PING: RFR: JDK-8151815: Could not parse core image with JSnap.

Wed Oct 18 06:34:13 UTC 2017

Hi David,

> I don't think we need the extra fields, just ensure the existing ones can't
> be accessed (other than by the tools) after destroy is called.

I've added PerfMemory::is_useable() to check whether we can access to
PerfMemory.
I think this webrev prevent to access to PerfMemory after destroy() call.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.06/

Thanks,

Yasumasa

2017-10-18 13:44 GMT+09:00 David Holmes <david.holmes at oracle.com>:
> On 18/10/2017 2:27 PM, Yasumasa Suenaga wrote:
>>
>> Hi David,
>>
>> 2017-10-18 12:55 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>>>
>>> On 18/10/2017 12:37 PM, Yasumasa Suenaga wrote:
>>>>
>>>>
>>>> Hi David,
>>>>
>>>>> With your changes you no longer null out _prologue so the assertion
>>>>> would
>>>>> now not fail and we'd proceed to access the deleted memory region!
>>>>
>>>>
>>>>
>>>> On Linux, PerfMemory::delete_memory_region() does not call munmap()
>>>> for PerfMemory.
>>>
>>>
>>>
>>> Perhaps not but there are still other actions that happen and the point
>>> is
>>> we should not be able to continue to use PerfMemory once it has been
>>> destroyed (even if the destruction is only logical).
>>
>>
>> I received same comment from Dmitry in the past, but we couldn't
>> decide how should we do.
>>
>>
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-May/019728.html
>>
>> In that discussion, I uploaded another webrev which adds other fields for
>> JSnap.
>> Is it suitable?
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>
>
> I don't think we need the extra fields, just ensure the existing ones can't
> be accessed (other than by the tools) after destroy is called.
>
>>
>>>>> I'm unclear why you no longer clear all the fields set during
>>>>> initialization?
>>>>
>>>>
>>>>
>>>> PerfMemory.java in jdk.hotspot.agent needs these field values.
>>>> `jhsdb jsnap --core` is failed if they are cleared.
>>>
>>>
>>>
>>> I'm not familiar with these tools. When do we produce a core file after
>>> calling PerfMemory::destroy ?
>>
>>
>> PerfMemory::destroy() is called before aborting.
>
>
> Ah - right. I assume we need to close off the perfdata file before we abort.
>
> Thanks,
> David
>
>
>> -----------------------
>> #0  perfMemory_exit ()
>>      at
>> /usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/share/vm/runtime/perfMemory.cpp:80
>> #1  0x00007f99b091c949 in os::shutdown ()
>>      at
>> /usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os/linux/vm/os_linux.cpp:1483
>> #2  0x00007f99b091c980 in os::abort (dump_core=<optimized out>)
>>      at
>> /usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os/linux/vm/os_linux.cpp:1503
>> #3  0x00007f99b0b689c3 in VMError::report_and_die (
>>      this=this at entry=0x7ffcacf40b50)
>>      at
>> /usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/share/vm/utilities/vmError.cpp:1060
>> #4  0x00007f99b0926f04 in JVM_handle_linux_signal (sig=sig at entry=11,
>>      info=info at entry=0x7ffcacf40df0, ucVoid=ucVoid at entry=0x7ffcacf40cc0,
>>      abort_if_unrecognized=abort_if_unrecognized at entry=1)
>>      at
>> /usr/src/debug/java-1.8.0-openjdk-1.8.0.144-7.b01.fc26.x86_64/openjdk/hotspot/src/os_cpu/linux_x86/vm/os_linux_x86.cpp:541
>> -----------------------
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>>>> But it seems to me that there are various checks of
>>>>> _prologue that should really be checking is_initialized() and/or
>>>>> is_destroyed() as a guard.
>>>>
>>>>
>>>>
>>>> Should I change all assertions for _prologue?
>>>
>>>
>>>
>>> Assertions and direct guards. Checking _prologue is a placeholder for the
>>> real check.
>>>
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> 2017-10-18 10:53 GMT+09:00 David Holmes <david.holmes at oracle.com>:
>>>>>
>>>>>
>>>>> Hi Yasumasa,
>>>>>
>>>>> By chance we ran into this bug which I analysed yesterday:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8189390
>>>>>
>>>>> We hit the assertion:
>>>>>
>>>>> #  Internal Error (/open/src/hotspot/share/runtime/perfMemory.cpp:216),
>>>>> pid=17874, tid=17875
>>>>> #  assert(_prologue != __null) failed: called before initialization
>>>>> #
>>>>>
>>>>> which is misleading because it can fail if called before
>>>>> initialization,
>>>>> or
>>>>> after PerfMemory::destroy has been called.
>>>>>
>>>>> With your changes you no longer null out _prologue so the assertion
>>>>> would
>>>>> now not fail and we'd proceed to access the deleted memory region!
>>>>>
>>>>> I'm unclear why you no longer clear all the fields set during
>>>>> initialization? But it seems to me that there are various checks of
>>>>> _prologue that should really be checking is_initialized() and/or
>>>>> is_destroyed() as a guard.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>
>>>>> On 16/10/2017 11:25 PM, Yasumasa Suenaga wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> PING:
>>>>>>
>>>>>> Could you review it?
>>>>>>
>>>>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2017/10/03 13:18, Yasumasa Suenaga wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I added gtest unit test case for this change in new webrev:
>>>>>>>
>>>>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.05/
>>>>>>>
>>>>>>> Could you review it?
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 2017-09-27 0:01 GMT+09:00 Yasumasa Suenaga <yasuenag at gmail.com>:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I uploaded new webrev to be adapted to jdk10/hs:
>>>>>>>>
>>>>>>>>      http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.04/
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2017/09/21 7:45, Yasumasa Suenaga wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> PING:
>>>>>>>>>
>>>>>>>>> Have you checked this issue?
>>>>>>>>>
>>>>>>>>>>>       http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2017/07/01 23:43, Yasumasa Suenaga wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> PING:
>>>>>>>>>>
>>>>>>>>>> Have you checked this issue?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2017/06/13 14:10, Yasumasa Suenaga wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> I want to discuss about JDK-8151815: Could not parse core image
>>>>>>>>>>> with
>>>>>>>>>>> JSnap.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> In last year, I found JSnap cannot parse coredump and I've sent
>>>>>>>>>>> review
>>>>>>>>>>> request for it as JDK-8151815. However it has not been reviewed
>>>>>>>>>>> yet
>>>>>>>>>>> [1].
>>>>>>>>>>>
>>>>>>>>>>> We've discussed about safety implementation, but we could not get
>>>>>>>>>>> consensus.
>>>>>>>>>>> IMHO all SA tools should be handled java processes and core
>>>>>>>>>>> images,
>>>>>>>>>>> and PerfCounter value is useful. So I fix this issue.
>>>>>>>>>>>
>>>>>>>>>>> I uploaded new webrev for this issue. I think this patch is
>>>>>>>>>>> safety
>>>>>>>>>>> because new flag PerfMemory::_destroyed guards double free, and
>>>>>>>>>>> all
>>>>>>>>>>> members in PerfMemory is accessible (they are not munmap'ed)
>>>>>>>>>>>
>>>>>>>>>>>       http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.03/
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Can you cooperate?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> Yasumasa
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> [1]
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019480.html
>>>>>>>>>>>
>>>>>>>>
>>>>>
>>>
>