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'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. ----------------------- #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 >>>>>>>>> >>>>>> >>> >
