PING: RFR: JDK-8151815: Could not parse core image with JSnap.

[David Holmes]

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'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 ?

> 
>> 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
>>>>>>>>
>>>>>
>>