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

[coleen.phillimore at oracle.com]

On 6/1/20 9:21 AM, David Holmes wrote:
> 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()?

Yeah.
>
>> 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));
>    }
>  }
>

I was thinking not having ResourceMark in a loop, like this:

diff --git a/src/hotspot/share/utilities/vmError.cpp 
b/src/hotspot/share/utilities/vmError.cpp
--- a/src/hotspot/share/utilities/vmError.cpp
+++ b/src/hotspot/share/utilities/vmError.cpp
@@ -792,12 +792,13 @@
    STEP("inspecting top of stack")

       // decode stack contents if possible
-     if (_verbose && _context && Universe::is_fully_initialized()) {
+     if (_verbose && _context && _thread && 
Universe::is_fully_initialized()) {
         frame fr = os::fetch_frame_from_context(_context);
         const int slots = 8;
         const intptr_t *start = fr.sp();
         const intptr_t *end = start + slots;
         if (is_aligned(start, sizeof(intptr_t)) && 
os::is_readable_range(start, end)) {
+         ResourceMark rm(_thread);
           st->print_cr("Stack slot to memory mapping:");
           for (int i = 0; i < slots; ++i) {
             st->print("stack at sp + %d slots: ", i);


The check for _thread seems silly because why would we have _context 
without _thread, but this seems useful for safety and doesn't really 
hurt anything.

Thanks,
Coleen


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