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

[gerard ziemski]

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