PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa Suenaga
yasuenag at gmail.com
Thu Oct 19 11:44:30 UTC 2017
Hi,
> I suggest we leave the volatile off for now and file a RFE to add volatile_static_field support to VMStructs and update later.
Okay. David or Serguei, could you file it?
>> 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).
> 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.
Sorry, I have mistake the spell of "usable".
I've fixed it in new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.08/
Can I list David and Serguei as Reviewer?
I will send a changeset to Serguei if it can.
Thanks,
Yasumasa
On 2017/10/19 19:41, David Holmes wrote:
> 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