Code review request: 7190089: NMT ON: NMT failed assertion on thread's stack base address

[David Holmes]

On 31/08/2012 3:58 AM, Coleen Phillimore wrote:
> This does look like a clean solution. I was concerned because
> initialize_thread() call was moved to record_stack_base_and_size() but
> it's really doing initialize_thread_stack() except the last two lines. I
> think the name should be changed so it looks more reasonable inside this
> stack-related function.

So basically we wanted to call initialize_thread() inside 
Thread::record_stack_base_and_size() rather than as part of 
Thread::initialize_thread_local_storage(). But by doing so we can no 
longer call Thread::current() (because TLS is not set up) so have to add 
a parameter to initialize_thread to pass the thread in - and this meant 
touching all the os_<os>_<arch>.cpp files plus the header. :(

I can't see a way to avoid passing in the thread as a parameter. But I 
do question why the only non-noop implementation of initialize_thread is 
in the OS specific but arch-neutral os_solaris.cpp, where all the no-op 
implementations are in the os_<os>_<arch>.cpp files ??? I'd rather see 
one implementation per OS than per os-arch pair. Not that this would 
make this changeset smaller.

David

>
> Thanks,
> Coleen
>
> On 8/30/2012 1:41 PM, Karen Kinnear wrote:
>> Zhengyu.
>>
>> Code looks good - and this looks like a clean solution. Also, thank
>> you for the additional
>> assertions and the renaming to stack_low_addr.
>>
>> Ship it!
>> Karen
>>
>> On Aug 30, 2012, at 1:11 PM, Zhengyu Gu wrote:
>>
>>> That will be great!
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>> On 8/30/2012 12:53 PM, Vladimir Kozlov wrote:
>>>> Looks good. If you want, you can push it into hotspot-comp since
>>>> runtime repo is closed already as I understand.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> Zhengyu Gu wrote:
>>>>> This is a Solaris only bug. When stack size limit is set to
>>>>> unlimited (ulimit -s unlimited), primordial thread's stack size has
>>>>> to be further adjusted before native memory tracker can record its
>>>>> base and size.
>>>>>
>>>>> CR: http://bugs.sun.com/view_bug.do?bug_id=7190089
>>>>> Webrev: http://cr.openjdk.java.net/~zgu/7190089/webrev.00/
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>