PING: RFR: JDK-8151815: Could not parse core image with JSnap.
David Holmes
david.holmes at oracle.com
Wed Oct 18 09:51:09 UTC 2017
On 18/10/2017 7:43 PM, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Should we use OrderAccess::load_acquire() to check _initialized and
> _destroyed?
Yes for _initialized.
> IMHO it do not need because initialize / destroy of PerfMemory seem not
> to be on multi-threaded.
Initialization would normally be single-threaded, but I suspect that may
be (or were in the past) possible ways to have it occur in different
threads - else we'd not need the release_store.
Destroy can be attempted by multiple threads I believe, if there are
multiple aborts for example.
Thanks,
David
>
> Thanks,
>
> Yasumasa.
>
>
> 2017/10/18 午後6:25 "serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>" <serguei.spitsyn at oracle.com
> <mailto:serguei.spitsyn at oracle.com>>:
>
> Hi Yasumasa,
>
> Sorry for a quite late participation.
>
> I looked at the previous webrevs and think that this one is much better.
>
> Some concern is if we need any kind of synchronization here, e.g. CAS.
> But it depends on the PerfMemory class usage.
>
> Should we make the static variables '_initialized' and '_destroyed'
> volatile?
>
> Also, the '_initialized' is set to 1 with:
> 159 OrderAccess::release_store(&_initialized, 1);
>
> Should we do the same to set the '_destroyed'?:
> 200 _destroyed = true;
>
>
> Thanks,
> Serguei
>
>
> On 10/18/17 00:39, Yasumasa Suenaga wrote:
>> Hi David,
>>
>> Thank you for your comment.
>> I uploaded new webrev:
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.07/
>> <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> <mailto: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/
>>>> <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> <mailto: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> <mailto: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
>>>>>> <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/
>>>>>> <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> <mailto: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
>>>>>>>>> <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/
>>>>>>>>>>> <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/
>>>>>>>>>>> <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> <mailto: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/
>>>>>>>>>>>> <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/
>>>>>>>>>>>>>>> <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/
>>>>>>>>>>>>>>> <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
>>>>>>>>>>>>>>> <http://mail.openjdk.java.net/pipermail/serviceability-dev/2016-April/019480.html>
>>>>>>>>>>>>>>>
>
More information about the serviceability-dev
mailing list