PING: RFR: JDK-8151815: Could not parse core image with JSnap.
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Oct 19 09:37:30 UTC 2017
Hi Yasumasa,
I see the problem.
As it occurred making these variables volatile is non-trivial.
But thank you a lot for trying!
I'd suggest to fall back to your previous approach as synchronization
was not there
in the first place, and it is not a part of the original issue you are
trying to fix
(if David or anyone else does not a simple solution).
But let's check if David does not object against it.
I will sponsor your fix after you send me a patch.
Thanks,
Serguei
On 10/18/17 20:21, Yasumasa Suenaga wrote:
> 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