PING: RFR: JDK-8151815: Could not parse core image with JSnap.
Yasumasa Suenaga
yasuenag at gmail.com
Thu Oct 19 13:43:15 UTC 2017
Sorry, I forgot the fix to use OrderAccess::load_acquire() in PerfMemory::is_initialized().
I fixed it in new webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.09/
Yasumasa
On 2017/10/19 21:24, David Holmes wrote:
> On 19/10/2017 9:44 PM, Yasumasa Suenaga wrote:
>> 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.
>
> Yes.
>
> Thanks,
> David
>
>>
>> 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