RFR (S): 8245509: Crash handler itself crashes when reporting Unsafe.putInt(0) crash

[David Holmes]

On 2/06/2020 8:45 am, coleen.phillimore at oracle.com wrote:
> On 6/1/20 6:36 PM, David Holmes wrote:
>> On 2/06/2020 7:21 am, coleen.phillimore at oracle.com wrote:
>>> On 6/1/20 3:57 PM, gerard ziemski wrote:
>>>> hi Coleen,
>>>>
>>>> On 6/1/20 1:33 PM, coleen.phillimore at oracle.com wrote:
>>>>> In rev2, in the second part, can you move the ResourceMark just out 
>>>>> of the loop?  Otherwise, looks good and I don't need to see another 
>>>>> webrev. 
>>>>
>>>> I thought the ResourceMark should go with the operation it's meant 
>>>> for, to make the code more self documenting.
>>
>> Yes but you don't have to put it right before the op especially in 
>> loops. I only had it in the loop because I was not applying this change:
>>
>> -     if (_verbose && _context && Universe::is_fully_initialized()) {
>> +     if (_verbose && _context && _thread && 
>> Universe::is_fully_initialized()) {
>>
>> which I do not agree with. We are now excluding printing this 
>> information if we don't have an attached thread. It was okay to 
>> exclude "printing register info" because the next section printed 
>> similar info for when we do not have a _thread. It isn't clear to me 
>> that is the case here.
>>
>> I would prefer not to print less information if we don't have to 
>> exclude it.
> 
> If there's no current thread (which is 100% unlikely in this case) it 
> doesn't make any sense to print it's stack.

Well we already try to do just that - if we crash while attaching the 
current thread for example. But in that case a bunch of error reporting 
steps break anyway, including this one:

Stack slot to memory mapping:
stack at sp + 0 slots:
[error occurred during error reporting (inspecting top of stack), id 
0xe0000000, Internal Error 
(/jdk-dev/open/src/hotspot/share/runtime/thread.hpp:855)]

So I guess we are not losing anything. :(

David

> Coleen
> 
>>
>> David
>> -----
>>
>>>>
>>>> Why does it need to go outside pf the loop? To minimize number of 
>>>> allocations?
>>>
>>> It sets an allocation marker in the Arena to allocate from in the 
>>> current thread.  It's a pretty cheap operation, and it's for error 
>>> handling, so it can go inside the loop.  Your change is fine.
>>>
>>> Coleen
>>>
>>>>
>>>>
>>>> cheers
>>>
>