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/ 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 >>>>>>>>>>> >>>>>>>> >>>>> >>> >
