PING: RFR: JDK-8151815: Could not parse core image with JSnap.
David Holmes
david.holmes at oracle.com
Wed Oct 18 07:09:22 UTC 2017
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