Code review request: CR 7141259 Native stack is missing in hs_err

[Karen Kinnear]

Looks fine. Ship it.

Thanks Zhengyu,
Karen

On Feb 8, 2012, at 10:07 PM, David Holmes wrote:

> I didn't see any comments from Keith so had to infer what the additional changes were.
> 
> Seems okay.
> 
> David
> 
> On 9/02/2012 4:23 AM, Zhengyu Gu wrote:
>> Hi,
>> 
>> As I mentioned in earlier email, please find revised webrev, which
>> incorporated suggestions from Karen and Keith.
>> 
>> http://cr.openjdk.java.net/~zgu/7141259/webrev.02/
>> 
>> Thanks,
>> 
>> -Zhengyu
>> 
>> On 2/8/2012 10:19 AM, Karen Kinnear wrote:
>>> Zhengyu,
>>> 
>>> The code looks like it will work.
>>> 
>>> I would appreciate a comment that we never cleanup the error handler
>>> decoder since it
>>> is only used on crashing.
>>> Just to be sure I understand - there are currently no callers of
>>> Decoder::shutdown(), i.e.
>>> no users of the shared decoder are in the system yet.
>>> Longer term, with additional cases I would recommend managing the
>>> scope of the decoder
>>> instance outside the decoder code itself.
>>> 
>>> thanks,
>>> Karen
>>> 
>>> On Feb 6, 2012, at 9:30 AM, Zhengyu Gu wrote:
>>> 
>>>> David,
>>>> 
>>>> I total agree that it is a bit of hack. The problem is that, decoder
>>>> actually hides behind os::dll_address_to_function_name(), which makes
>>>> it less flexible in design. Unless we are willing to mark it as
>>>> multi-thread unsafe, and manage decoder instance and lock outside the
>>>> scope. Native memory tracking has such flexibility, I think that the
>>>> real issue is how to redesign dll_address_to_function_name() API.
>>>> 
>>>> 
>>>> Thanks,
>>>> 
>>>> -Zhengyu
>>>> 
>>>> On 2/6/2012 3:14 AM, David Holmes wrote:
>>>>> Hi Zhengyu,
>>>>> 
>>>>> On 4/02/2012 8:12 AM, Zhengyu Gu wrote:
>>>>>> Thanks for reviewing. I think I made wrong assumption that the
>>>>>> thread is
>>>>>> the last running thread.
>>>>>> 
>>>>>> 803 // This should be the only running thread, decoder can safely
>>>>>> run in
>>>>>> 804 // single threaded mode without acquiring lock
>>>>>> 805 Decoder::set_single_threaded(true);
>>>>>> 806
>>>>>> 
>>>>>> To avoid locking in error handler, the only solution I can think
>>>>>> of, is
>>>>>> to have its own private decoder.
>>>>>> 
>>>>>> Please see revised webrev:
>>>>>> http://cr.openjdk.java.net/~zgu/7141259/webrev.01/
>>>>> This seems functionally ok. It is a bit strange to have a static API
>>>>> built on the assumption of a singleton instance now having to
>>>>> special case "is this the VM error thread" and use more than once
>>>>> instance, but I think changing that requires revisiting the whole
>>>>> Decoder framework.
>>>>> 
>>>>> David
>>>>> -----
>>>>> 
>>>>>>> Later when there are multiple users of the Decoder I don't know how
>>>>>>> you will fix this because those other users can run concurrently with
>>>>>>> report_and_die(). That said I'm unclear what it is about the Decoder
>>>>>>> that requires locking - what state does it maintain? Is there a
>>>>>>> reason
>>>>>>> not to create a Decoder only when needed?
>>>>>>> 
>>>>>> Decoder is created on demand, but would prefer to be shared, since it
>>>>>> can consume noticeable amount memory on Unix platform, by loading Elf
>>>>>> files' symbol tables and string tables into memory (not impact on
>>>>>> Windows). The lock is needed to protect a linked list structure in Elf
>>>>>> decoder, or current file position.
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> -Zhengyu
>>>>>>> ---
>>>>>>> 
>>>>>>> In decoder.cpp you can still use MutexLockerEx and as it will take a
>>>>>>> null mutex and just act as a no-op eg:
>>>>>>> 
>>>>>>> MutexLockerEx locker( _single_thread ? NULL : _decoder_lock, true);
>>>>>>> 
>>>>>>> Cheers,
>>>>>>> David
>>>>>>> -----
>>>>>>> 
>>>>>>>> Please review
>>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/7141259/webrev.00/
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> 
>>>>>>>> -Zhengyu
>>>>>>>>