PING: RFR: JDK-8151815: Could not parse core image with JSnap.
David Holmes
david.holmes at oracle.com
Thu Oct 19 10:41:05 UTC 2017
Hi Serguei, Yasumasa,
I suggest we leave the volatile off for now and file a RFE to add
volatile_static_field support to VMStructs and update later.
I don't think trying to introduce locking would be a good idea as it
would likely lead to deadlocks when a crash occurs. This could also be
investigated as a future RFE if desired.
Thanks,
David
On 19/10/2017 7:37 PM, serguei.spitsyn at oracle.com wrote:
> 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