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

[coleen.phillimore at oracle.com]

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.
Thanks!
Coleen

On 6/1/20 12:08 PM, gerard ziemski wrote:
> Thank you Coleen, David for your feedback!
>
>
> On 6/1/20 9:06 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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.
>
> I prepared new webrev based on the feedback. How does that look?
>
> bug link at https://bugs.openjdk.java.net/browse/JDK-8245509
> webrev at http://cr.openjdk.java.net/~gziemski/8245509_rev2
> Mach5 hs_tier1,2,3,4,5 in progress....
>
>
> cheers
>