PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa Suenaga
yasuenag at gmail.com
Thu Oct 19 03:21:26 UTC 2017
Sorry, I have mistake.
But I cannot compile yet:
diff -r 3e7702cd3f19 src/hotspot/share/runtime/vmStructs.cpp
--- a/src/hotspot/share/runtime/vmStructs.cpp Thu Sep 07 15:40:20 2017 +0200
+++ b/src/hotspot/share/runtime/vmStructs.cpp Thu Oct 19 12:21:11 2017 +0900
@@ -578,7 +578,7 @@
static_field(PerfMemory, _top, char*) \
static_field(PerfMemory, _capacity, size_t) \
static_field(PerfMemory, _prologue, PerfDataPrologue*) \
- static_field(PerfMemory, _initialized, jint) \
+ static_field(PerfMemory, _initialized, volatile jint) \
--------------
In file included from /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:104:0:
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:58: error: invalid conversion from 'volatile void*' to 'void*' [-fpermissive]
{ QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:6: note: in expansion of macro 'GENERATE_STATIC_VM_STRUCT_ENTRY'
static_field(PerfMemory, _initialized, volatile jint) \
^~~~~~~~~~~~
/home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3: note: in expansion of macro 'VM_STRUCTS'
VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
^
gmake[3]: *** [lib/CompileJvm.gmk:210: /home/ysuenaga/OpenJDK/jdk10-hs/build/linux-x86_64-normal-server-fastdebug/hotspot/variant-server/libjvm/objs/vmStructs.o] Error 1
gmake[2]: *** [make/Main.gmk:266: hotspot-server-libs] Error 2
ERROR: Build failed for target 'images' in configuration 'linux-x86_64-normal-server-fastdebug' (exit code 2)
--------------
On 2017/10/19 12:18, Yasumasa Suenaga wrote:
> Hi Serguei,
>
>> Would the below work? :
>>
>> 578 static_field(PerfMemory, _initialized, volatile jint) \
>>
>> It'd be similar to this non-static case:
>> 362 nonstatic_field(ConstantPoolCacheEntry, _f1, volatile Metadata*) \
>
> I got error messages as below:
>
> ---------------
> In file included from /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:104:0:
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39: error: expected unqualified-id before 'volatile'
> static_field(PerfMemory, volatile _initialized, jint) \
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69: note: in definition of macro 'GENERATE_STATIC_VM_STRUCT_ENTRY'
> { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },
> ^~~~~~~~~
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3: note: in expansion of macro 'VM_STRUCTS'
> VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39: error: expected '}' before 'volatile'
> static_field(PerfMemory, volatile _initialized, jint) \
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69: note: in definition of macro 'GENERATE_STATIC_VM_STRUCT_ENTRY'
> { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },
> ^~~~~~~~~
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3: note: in expansion of macro 'VM_STRUCTS'
> VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:39: error: expected '}' before 'volatile'
> static_field(PerfMemory, volatile _initialized, jint) \
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:69: note: in definition of macro 'GENERATE_STATIC_VM_STRUCT_ENTRY'
> { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },
> ^~~~~~~~~
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3: note: in expansion of macro 'VM_STRUCTS'
> VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.hpp:168:79: error: expected declaration before '}' token
> { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },
> ^
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:581:6: note: in expansion of macro 'GENERATE_STATIC_VM_STRUCT_ENTRY'
> static_field(PerfMemory, volatile _initialized, jint) \
> ^~~~~~~~~~~~
> /home/ysuenaga/OpenJDK/jdk10-hs/src/hotspot/share/runtime/vmStructs.cpp:2934:3: note: in expansion of macro 'VM_STRUCTS'
> VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY,
> ^
> gmake[3]: *** [lib/CompileJvm.gmk:210: /home/ysuenaga/OpenJDK/jdk10-hs/build/linux-x86_64-normal-server-fastdebug/hotspot/variant-server/libjvm/objs/vmStructs.o] Error 1
> gmake[2]: *** [make/Main.gmk:266: hotspot-server-libs] Error 2
>
> ERROR: Build failed for target 'images' in configuration 'linux-x86_64-normal-server-fastdebug' (exit code 2)
> ---------------
>
>
> I changed as below:
> ---------------
> diff -r 3e7702cd3f19 src/hotspot/share/runtime/perfMemory.cpp
> --- a/src/hotspot/share/runtime/perfMemory.cpp Thu Sep 07 15:40:20 2017 +0200
> +++ b/src/hotspot/share/runtime/perfMemory.cpp Thu Oct 19 12:15:30 2017 +0900
> @@ -51,8 +51,9 @@
> char* PerfMemory::_end = NULL;
> char* PerfMemory::_top = NULL;
> size_t PerfMemory::_capacity = 0;
> -jint PerfMemory::_initialized = false;
> +volatile jint PerfMemory::_initialized = 0;
> PerfDataPrologue* PerfMemory::_prologue = NULL;
> +volatile bool PerfMemory::_destroyed = false;
>
> --- a/src/hotspot/share/runtime/perfMemory.hpp Thu Sep 07 15:40:20 2017 +0200
> +++ b/src/hotspot/share/runtime/perfMemory.hpp Thu Oct 19 12:15:30 2017 +0900
> @@ -113,13 +113,15 @@
> */
> class PerfMemory : AllStatic {
> friend class VMStructs;
> + friend class PerfMemoryTest;
> private:
> static char* _start;
> static char* _end;
> static char* _top;
> static size_t _capacity;
> static PerfDataPrologue* _prologue;
> - static jint _initialized;
> + static volatile jint _initialized;
> + static volatile bool _destroyed;
>
> diff -r 3e7702cd3f19 src/hotspot/share/runtime/vmStructs.cpp
> --- a/src/hotspot/share/runtime/vmStructs.cpp Thu Sep 07 15:40:20 2017 +0200
> +++ b/src/hotspot/share/runtime/vmStructs.cpp Thu Oct 19 12:15:30 2017 +0900
> @@ -578,7 +578,7 @@
> static_field(PerfMemory, _top, char*) \
> static_field(PerfMemory, _capacity, size_t) \
> static_field(PerfMemory, _prologue, PerfDataPrologue*) \
> - static_field(PerfMemory, _initialized, jint) \
> + static_field(PerfMemory, volatile _initialized, jint) \
> ---------------
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2017/10/19 6:18, serguei.spitsyn at oracle.com wrote:
>> On 10/18/17 06:51, Yasumasa Suenaga wrote:
>>> Hi David, Serguei,
>>>
>>>> because as soon as we have checked is_usable() and abort happening in another thread may have changed that by calling destroy.
>>>>
>>>> This code is basically broken if we hit an abort path instead of a normal VM shutdown.
>>>
>>> Can we use MutexLocker for initialize() and destroy() ?
>>>
>>>
>>> I've tried to fix about your comments, but I have an issue about volatile.
>>> PerfMemory.java depends on PerfMemory::_initialized. However VMStructs cannot handle static volatile variables.
>>> I think two approaches as below:
>>>
>>>
>>> 1. Remove _initialized check from PerfMemory.java
>>> SA will throw UnmappedAddressException if JSnap try to access invalid address including uninitialized memory.
>>>
>>> 2. Add static volatile support to VMStructs
>>>
>>>
>>> Which should we do?
>>> 1. is easy to fix. But 2. might be right way...
>>
>> Would the below work? :
>>
>> 578 static_field(PerfMemory, _initialized, volatile jint) \
>>
>> It'd be similar to this non-static case:
>> 362 nonstatic_field(ConstantPoolCacheEntry, _f1, volatile Metadata*) \
>>
>>
>> Thanks,
>> Serguei
>>
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2017/10/18 21:34, David Holmes wrote:
>>>> Just to clarify ...
>>>>
>>>> On 18/10/2017 10:28 PM, David Holmes wrote:
>>>>> On 18/10/2017 8:26 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> Thank you for jumping to this review and helping Yasumasa to sort it out!
>>>>>> I've just discovered that this issue was already on the table for several months without a significant progress.
>>>>>>
>>>>>>
>>>>>> On 10/18/17 02:48, David Holmes wrote:
>>>>>>> Hi Serguei
>>>>>>>
>>>>>>> On 18/10/2017 7:25 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> 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?
>>>>>>>
>>>>>>> For good measure - yes.
>>>>>>>
>>>>>>>> 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;
>>>>>>>
>>>>>>> There is a benign initialization race but we need the release_store to ensure all the data fields can be read if _initialized is seen as true. But what is missing is a load_acquire() in is_initialized() to ensure we synchronize with that store!
>>>>>>
>>>>>> Yes, I noticed that the load_acquire() is missed. :|
>>>>>>
>>>>>>>
>>>>>>> There is also a potential for a destruction race (if multiple aborts happens concurrently in different threads) but that also seems benign. In this case there is no data being set so the store to _destroyed does not need to be a release_store.
>>>>>>
>>>>>> I'm not convinced yet this is benign as the PerfMemory::destroy() has this call:
>>>>>> 197 delete_memory_region();
>>>>>
>>>>> Yes though most of its work ends up being no-ops.
>>>>>
>>>>>>
>>>>>> Now, I started thinking about the asserts that call the is_useable().
>>>>>> Should they be returns instead?
>>>>>
>>>>> I think this is a somewhat confused chunk of code. It's only fractionally thread-safe yet once in use could be in use concurrently with an aborting thread that calls destroy(). I don't think there is any simple fix for this. If we're in the process of crashing does it really matter if we trigger a secondary crash due to this?
>>>>
>>>> It doesn't matter if we do:
>>>>
>>>> assert(is_usable(),...);
>>>> // continue
>>>>
>>>> or
>>>>
>>>> if (!is_usable()) return;
>>>> // continue
>>>>
>>>> because as soon as we have checked is_usable() and abort happening in another thread may have changed that by calling destroy.
>>>>
>>>> This code is basically broken if we hit an abort path instead of a normal VM shutdown.
>>>>
>>>> David
>>>> -----
>>>>
>>>>> The problems with this code go way beyond what Yasumasa is trying to address with the JSnap problem and I would not want to put it back on him to try and come up with an overall solution.
>>>>>
>>>>>> Then the is_destroyed() would better to have the load_acquire().
>>>>>
>>>>> You could add a load_acquire and do the store_release. It certainly would not hurt, but I don't think it would actually benefit anything either.
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>> Just interested to know what do you think on this.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> 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>:
>>>>>>>>>> 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>:
>>>>>>>>>>>> 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>:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 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>:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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>:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>
>>>>>>
>>
More information about the serviceability-dev
mailing list