PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa Suenaga
yasuenag at gmail.com
Wed Oct 18 07:39:37 UTC 2017
Hi David,
Thank you for your comment.
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.07/
Serguei, please comment about this :-)
Yasumasa
2017-10-18 16:09 GMT+09:00 David Holmes <david.holmes at oracle.com>:
> Hi Yasumasa,
>
> On 18/10/2017 4:34 PM, Yasumasa Suenaga wrote:
>>
>> 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/
>
>
> This:
>
> 90 void PerfMemory::initialize() {
> 91
> 92 if (_prologue != NULL)
> 93 // initialization already performed
> 94 return;
>
> shouldn't check _prologue, but is_initialized().
>
> 213 assert(is_useable(), "called before initialization");
>
> -> "called before init or after destroy"
>
> Could add a similar assert in PerfMemory::mark_updated().
>
> Let's see what Serguei thinks. :)
>
>
> Thanks,
> David
>
>>
>> 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
>>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
More information about the serviceability-dev
mailing list