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

[Zhengyu Gu]

Karen,

Thanks for reviewing. I am incorporating Keith's and your comments, will 
send out a revision later.


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