RFR: 8252921: NMT overwrite memory type for region assert when building dynamic archive

[David Holmes]

Hi Zhengyu,

On 16/09/2020 9:28 pm, Zhengyu Gu wrote:
> On Wed, 16 Sep 2020 02:49:45 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Thread stack is currently unregistered with NMT in Thread's destructor. Apparently, only Java thread invokes destructor
>>> before thread exits.  For NonJavaThread, e.g. ConcurrentGCThread, thread may exit while its "Thread" object continues
>>> alive, therefore, its thread stack is still "alive" from NMT perspective. Once thread exits, the virtual memory for the
>>> thread stack can be reserved again, that confused NMT.  The solution is to move thread stack unregistration code to
>>> post_run() method.
>>
>> src/hotspot/share/runtime/thread.hpp line 762:
>>
>>> 760:  public:
>>> 761:   // Stack overflow support
>>> 762:   address stack_base() const           { return _stack_base; }
>>
>> Why did you remove the assertion? We want the assertion in general to ensure there are no improper uses of stack_base().
> 
> We now reset NonJavaThread's stack base and size,  so _stack_base == NULL is possible, e.g. when generating hs_err
> report, as we should now see empty stack for "G1 Main Marker" thread in original bug report.

Resetting the stack base and size seems pointless given we follow that 
immediately with

Thread::clear_thread_current();

And once there is no thread-current for this thread its base and size 
can never be queried, so I don't think this change, or the change to the 
assertion in stack_base() is needed.

> You do have a valid point for the assertion. I think following assertion is more precise:
> 
> assert(_stack_base != NULL || Thread::current() != NULL, "Sanity check");
> 
> What you think?

That assertion doesn't make sense to me as the stack_base() being 
queried doesn't have to belong to the current thread.

Thanks,
David
-----

> Thanks
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/185
>