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

[coleen.phillimore at oracle.com]

On 6/1/20 2:44 AM, David Holmes wrote:
>
>
> On 30/05/2020 1:55 am, gerard ziemski wrote:
>> hi Coleen, David,
>>
>> Thank you for the reviews.
>>
>>
>> On 5/28/20 6:59 PM, David Holmes wrote:
>>> Hi Gerard,
>>>
>>> On 29/05/2020 3:36 am, gerard ziemski wrote:
>>>> hi all,
>>>>
>>>> Please review this small and simple fix that adds ResourceMark to 
>>>> crash handler when printing register info, without which can 
>>>> trigger fatal error if the contents of a register happens to be an 
>>>> oop, which uses as_C_string(), that in turn 
>>>> calls ResourceArea::allocate_bytes(), which requires a ResourceMark.
>>>>
>>>> If thread is not available, we do not attempt to print anything at 
>>>> this step (i.e. print_register_info()), but the very next step 
>>>> (i.e. print_context()) will always print the registers as HEX values.
>>>>
>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8245509
>>>> webrev at http://cr.openjdk.java.net/~gziemski/8245509_rev1
>>>> passes Mach5 hs_tier1,2,3,4,5
>>>
>>> This fixes the current problem and can be pushed.
>>>
>>> But as Coleen notes in the bug report there are other callers of 
>>> os::print_location that might also need a ResourceMark, and based on 
>>> your stack trace it seems it is Universe::heap()->print_location 
>>> that is the true source of the problem and where the ResourceMark 
>>> perhaps should be? So I think we need a follow up bug to fix the 
>>> broader problem.
>>
>> Thank you David for pointing this out, I misread Coleen's comment - I 
>> thought the "(and one with one)" referred to the "thread" argument of 
>> ResourceMark, not ResourceMark itself.
>>
>> So I think BlockLocationPrinter::print_location() would be the actual 
>> place that needs the ResourceMark (without the thread argument), is 
>> that correct?
>
> For G1 that is what I believe Universe::heap()->print_location will 
> end up calling:
>
> bool G1CollectedHeap::print_location(outputStream* st, void* addr) 
> const {
>   return BlockLocationPrinter<G1CollectedHeap>::print_location(st, addr);
> }
>
> but it in turn is calling:
>
>  o->print_on(st);
>
> where o is an Oop, and eventually the problem surfaces in 
> Klass::oop_print_on. So that seems to be where the ResourceMark should 
> be unless we have designated it something where the caller is expected 
> to have the RM ... which it is, and the caller in this case is 
> InstanceKlass::oop_print_on which neither has a ResourceMark nor 
> states that a RM is needed in the caller. So arguably that is where 
> the RM should be placed.

This is not the place to put it because this is only one place that all 
the GCs eventually dispatches to for the 
Universe::heap()->print_location() call.
>
> I'd vote for putting the ResourceMark as far down the call chain as 
> possible, but Coleen deals with this code more often than I and there 
> may be a reason to move it higher to e.g. 
> BlockLocationPrinter::print_location as you suggested.

Putting the ResourceMarks down as far as they can go can mess up 
logging.  There's an issue to examine this. 
https://bugs.openjdk.java.net/browse/JDK-8244999

In general, any function that takes an outputStream should have the 
ResourceMark in its caller.

Looking again, I like where Gerard has the ResourceMark.  The only 
callers that don't have ResourceMark are for calls that you make within 
the debugger (in debug.cpp).  I haven't run into that problem.  If we do 
run into that, we can add them later.

Gerard, can you do the same treatment (check for _thread non-null, and 
add ResourceMark) to this step too, which calls print_location?

   STEP("inspecting top of stack")

Then, I think we should limit this change to solve the problem that we 
keep seeing.

Thanks,
Coleen

>
> Cheers,
> David
> -----
>
>>
>>
>> cheers
>>