PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa Suenaga
yasuenag at gmail.com
Wed Oct 18 09:43:20 UTC 2017
Hi Serguei,
Should we use OrderAccess::load_acquire() to check _initialized and
_destroyed?
IMHO it do not need because initialize / destroy of PerfMemory seem not to
be on multi-threaded.
Thanks,
Yasumasa.
2017/10/18 午後6:25 "serguei.spitsyn at oracle.com" <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/
>
> Serguei, please comment about this :-)
>
>
> Yasumasa
>
>
>
> 2017-10-18 16:09 GMT+09:00 David Holmes <david.holmes at oracle.com> <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/
>
>
>
> 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> <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> <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
>
> 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 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> <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
>
> 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 <yasuenag at gmail.com> <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/
>
>
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20171018/95e4c2ec/attachment-0001.html>
More information about the serviceability-dev
mailing list