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