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

[serguei.spit...@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/ Serguei, please comment about this :-) Yasumasa 2017-10-18 16:09 GMT+09:00 David Holmes <david.hol...@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.hol...@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.hol...@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@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@entry=11, info=info@entry=0x7ffcacf40df0, ucVoid=ucVoid@entry=0x7ffcacf40cc0, abort_if_unrecognized=abort_if_unrecognized@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.hol...@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 <yasue...@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