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

[David Holmes]

On 17/09/2020 11:18 am, Zhengyu Gu wrote:
> On Thu, 17 Sep 2020 00:46:33 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:
>>>> 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.
> 
> In regular case, yes. But there is an exception, during error reporting (Threads::print_on_error()).
> In the original bug report, "G1 Main Marker" thread already exited and should not have stack, but hs_err file shows:
> 
> 0x000000a47fdee080 ConcurrentGCThread "G1 Main Marker" [stack: 0x000000a41cf40000,0x000000a41d040000] [id=17068]

In the original bug report the crash is in the VMThread! Subsequent 
snippets from Ioi show something happening with a G1 thread but I can't 
see the actual hs_err files for those.

But if hs_err shows a stack it means that _thread was non-NULL, which 
means that Thread::current() was non-NULL, which means we haven't 
executed the Thread::clear_thread_current() nor cleared the stack_base 
or size. The only time there could be a problem is if we were to crash 
in Thread::clear_thread_current() itself - which is unlikely enough that 
I am not concerned about that one extreme case versus the change being made.

David
-----


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