PING: RFR: JDK-8151815: Could not parse core image with JSnap.
David Holmes
david.holmes at oracle.com
Thu Oct 19 12:24:57 UTC 2017
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