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

[David Holmes]

Hi Coleen,

On 1/06/2020 11:00 pm, coleen.phillimore at oracle.com wrote:
> 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

Okay ...

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

... so for this is a chain of calls that take an outputStream, you are 
saying the RM should be at the very start of the chain. But in doing 
that we have to know that "st" is not resource-based and won't be 
affected by this ResourceMark - else we hit JDK-8244999.

> 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.

You mean the other callers of os::print_location()?

> 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")

as in:

  for (int i = 0; i < slots; ++i) {
    st->print("stack at sp + %d slots: ", i);
    if (_thread) {
      ResourceMark rm(_thread);
      os::print_location(st, *(start + i));
    } else {
      os::print_location(st, *(start + i));
    }
  }

?

Thanks,
David

> Then, I think we should limit this change to solve the problem that we 
> keep seeing.
> 
> Thanks,
> Coleen
> 
>>
>> Cheers,
>> David
>> -----
>>
>>>
>>>
>>> cheers
>>>
>