[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: PING: RFR: JDK-8151815: Could not parse core image with JSnap.
From: Yasumasa Suenaga <yasuenag () gmail ! com>
Date: 2016-05-25 11:58:25
Message-ID: cd2ed8b7-bbb7-53fa-3b31-c054aaeae0ac () gmail ! com
[Download RAW message or body]
Hi Dmitry,
>>> I understand the problem, but I'm concerned of storing pointers to
>>> invalid memory region just for coredump. Sorry!
The latest webrev [1] holds pointers for PerfMemory in _saved_* fields.
These pointers are valid because they are not unmap'ed.
I think that it is a bug not to parse PerfMemory in coredump.
SA tools should be able to handle data in coredump.
Can we fix this issue?
Thanks,
Yasumasa
[1] http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
On 2016/05/06 22:39, Yasumasa Suenaga wrote:
> Dmitry,
>
>>> The solution might be in another areas, e.g. print value of performance
>>> counters to hs_err.log on crash.
>>
>> If so, we have to use native debugger.
>> For Java developers and troubleshooters, I want to support this feature
>> with JDK tool.
>
> If we encounter native stack overflow error, we cannot get hs_err log.
> I posted a bug report for this issue as JDK-7109520, but it was rejected.
>
> In all crash case, we might collect core image.
> If I collect it, I want to check performance counter in it.
>
> Thus I want to merge this patch to JDK 9.
> Is it difficult?
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/05/06 21:42, Yasumasa Suenaga wrote:
>> Hi Dmitry,
>>
>> On 2016/05/06 21:22, Dmitry Samersoff wrote:
>>> Yasumasa,
>>>
>>> I understand the problem, but I'm concerned of storing pointers to
>>> invalid memory region just for coredump. Sorry!
>>
>> I do not think that it is NOT invalid memory in webrev.02 .
>> PerfMemory is mmap'ed, and it is not unmap'ed.
>>
>> In addition, webrev.02 adds several pointers to store addresses which are
>> related to PerfMemory.
>> It is not disruptive against current memory management which is related to
>> PerfMemory.
>>
>> Coredump will include all virtual memory and thread contexts.
>> If pointers ( _saved_* in webrev.02) are invalid, it is not affect JVM behavior,
>> but JSnap will fail.
>> (This pointer is referred from JSnap only.)
>>
>>
>>> The solution might be in another areas, e.g. print value of performance
>>> counters to hs_err.log on crash.
>>
>> If so, we have to use native debugger.
>> For Java developers and troubleshooters, I want to support this feature
>> with JDK tool.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>> -Dmitry
>>>
>>>
>>> On 2016-05-06 14:42, Yasumasa Suenaga wrote:
>>>> PING: Have you ever reviewed it?
>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>>>>
>>>> I've sent review request to parse core image with JSnap.
>>>> If you have unclear point about this change, please tell me.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2016/04/20 21:21, Dmitry Samersoff wrote:
>>>>> Yasumasa,
>>>>>
>>>>> I need some more time to check what is happening with performance
>>>>> counters and what side effect this fix can have.
>>>>>
>>>>> Sorry about it.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2016-04-20 15:14, Yasumasa Suenaga wrote:
>>>>>> PING: Could you review it?
>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>>>>>>
>>>>>> This changes is based on jdk9/hs-rt.
>>>>>> But I confirmed this patch works fine on jdk9/hs .
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> On 2016/04/13 22:22, Yasumasa Suenaga wrote:
>>>>>>> Hi Dmitry,
>>>>>>>
>>>>>>> Thank you for your comment.
>>>>>>> I uploaded new webrev. Could you review again?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2016/04/13 21:41, Dmitry Samersoff wrote:
>>>>>>>> Yasumasa,
>>>>>>>>
>>>>>>>> Yes. It's better.
>>>>>>>>
>>>>>>>> Please place all _saved_* declarations together and add a comment
>>>>>>>> explaining what is the purpose of this variables.
>>>>>>>>
>>>>>>>> -Dmitry
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2016-04-13 15:20, Yasumasa Suenaga wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Could you review and sponsor it?
>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
>>>>>>>>>
>>>>>>>>> If it is not accepted, I think that we can add new field to
>>>>>>>>> PerfMemory
>>>>>>>>> for using in JSnap:
>>>>>>>>> -----------------------
>>>>>>>>> diff -r 4823056a5bbd src/share/vm/runtime/perfMemory.cpp
>>>>>>>>> --- a/src/share/vm/runtime/perfMemory.cpp Tue Apr 12 09:08:48
>>>>>>>>> 2016 +0000
>>>>>>>>> +++ b/src/share/vm/runtime/perfMemory.cpp Wed Apr 13 14:18:15
>>>>>>>>> 2016 +0900
>>>>>>>>> @@ -45,11 +45,16 @@
>>>>>>>>> UINT_CHARS + 1;
>>>>>>>>>
>>>>>>>>> char* PerfMemory::_start = NULL;
>>>>>>>>> +char* PerfMemory::_saved_start = NULL;
>>>>>>>>> char* PerfMemory::_end = NULL;
>>>>>>>>> +char* PerfMemory::_saved_end = NULL;
>>>>>>>>> char* PerfMemory::_top = NULL;
>>>>>>>>> +char* PerfMemory::_saved_top = NULL;
>>>>>>>>> size_t PerfMemory::_capacity = 0;
>>>>>>>>> +size_t PerfMemory::_saved_capacity = 0;
>>>>>>>>> jint PerfMemory::_initialized = false;
>>>>>>>>> PerfDataPrologue* PerfMemory::_prologue = NULL;
>>>>>>>>> +PerfDataPrologue* PerfMemory::_saved_prologue = NULL;
>>>>>>>>>
>>>>>>>>> void perfMemory_init() {
>>>>>>>>>
>>>>>>>>> @@ -103,6 +108,8 @@
>>>>>>>>>
>>>>>>>>> // allocate PerfData memory region
>>>>>>>>> create_memory_region(capacity);
>>>>>>>>> + _saved_start = _start;
>>>>>>>>> + _saved_capacity = _capacity;
>>>>>>>>>
>>>>>>>>> if (_start == NULL) {
>>>>>>>>>
>>>>>>>>> @@ -132,8 +139,11 @@
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> _prologue = (PerfDataPrologue *)_start;
>>>>>>>>> + _saved_prologue = _prologue;
>>>>>>>>> _end = _start + _capacity;
>>>>>>>>> + _saved_end = _end;
>>>>>>>>> _top = _start + sizeof(PerfDataPrologue);
>>>>>>>>> + _saved_top = _top;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> assert(_prologue != NULL, "prologue pointer must be
>>>>>>>>> initialized");
>>>>>>>>> -----------------------
>>>>>>>>>
>>>>>>>>> If it is better, I will upload a webrev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2016/04/06 12:42, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> Thanks for your comment.
>>>>>>>>>>
>>>>>>>>>> I uploaded a new webrev. Could you review again?
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/
>>>>>>>>>>
>>>>>>>>>> On 2016/04/06 0:31, Dmitry Samersoff wrote:
>>>>>>>>>>> Yasumasa,
>>>>>>>>>>>
>>>>>>>>>>> 1. It's better to change JSnap code to produce meaningful error
>>>>>>>>>>> message
>>>>>>>>>>> instead of NPE.
>>>>>>>>>>
>>>>>>>>>> I added null check and set message to PerfMemory.java .
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 2. We should check that no other consumer of perf counters rely on
>>>>>>>>>>> the
>>>>>>>>>>> fact it's NULL after call to destroy(). I'm not sure that this
>>>>>>>>>>> part of
>>>>>>>>>>> the fix is not dangerous.
>>>>>>>>>>
>>>>>>>>>> I added new flag (_destroyed) in PerfMemory class.
>>>>>>>>>> This flag set true at PerfMemory::destroy().
>>>>>>>>>>
>>>>>>>>>> _start, _end, and more field which I removed from destroy() are
>>>>>>>>>> private member of
>>>>>>>>>> PerfMemory. I implemented that the getter functions of them check
>>>>>>>>>> _destroyed flag,
>>>>>>>>>> and returns same value before this change.
>>>>>>>>>>
>>>>>>>>>> So I think this change does not affect other place.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>> On 2016-03-29 15:02, Yasumasa Suenaga wrote:
>>>>>>>>>>>> PING: Could you review it?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2016/03/22 21:24, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> PING: Could you review it?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>> 2016/03/14 23:39 "Yasumasa Suenaga" <yasuenag@gmail.com
>>>>>>>>>>>>> <mailto:yasuenag@gmail.com>>:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> When I use `jhsdb jsnap` to get PerfCounter from core
>>>>>>>>>>>>> images, I
>>>>>>>>>>>>> encountered NPE:
>>>>>>>>>>>>> -------------
>>>>>>>>>>>>> Exception in thread "main" java.lang.NullPointerException
>>>>>>>>>>>>> at sun.jvm.hotspot.tools.JSnap.run(JSnap.java:45)
>>>>>>>>>>>>> at
>>>>>>>>>>>>> sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:260)
>>>>>>>>>>>>> at sun.jvm.hotspot.tools.Tool.start(Tool.java:223)
>>>>>>>>>>>>> at
>>>>>>>>>>>>> sun.jvm.hotspot.tools.Tool.execute(Tool.java:118)
>>>>>>>>>>>>> at sun.jvm.hotspot.tools.JSnap.main(JSnap.java:67)
>>>>>>>>>>>>> at
>>>>>>>>>>>>> sun.jvm.hotspot.SALauncher.runJSNAP(SALauncher.java:352)
>>>>>>>>>>>>> at
>>>>>>>>>>>>> sun.jvm.hotspot.SALauncher.main(SALauncher.java:404)
>>>>>>>>>>>>> -------------
>>>>>>>>>>>>>
>>>>>>>>>>>>> PerfMemory::destroy() clears all members which are used in
>>>>>>>>>>>>> JSnap.
>>>>>>>>>>>>> Thus NPE is occurred.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I uploaded webrev for this issue.
>>>>>>>>>>>>> Could you review it?
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>> I cannot access JPRT.
>>>>>>>>>>>>> So I need a Sponsor.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>
>>>>>
>>>
>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic