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

[David Holmes]

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