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

[coleen.phillimore at oracle.com]

On 6/1/20 11:55 AM, Thomas Stüfe wrote:
> Hi,
>
> On Mon, Jun 1, 2020 at 3:00 PM <coleen.phillimore at oracle.com 
> <mailto: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
>
>     In general, any function that takes an outputStream should have the
>     ResourceMark in its caller.
>
>
> Just a short remark from the side, I am quite sure this is not an 
> issue anymore. outputStream classes should not use resource area anymore:
>
> https://bugs.openjdk.java.net/browse/JDK-8224193
> https://bugs.openjdk.java.net/browse/JDK-8181917

I thought even with the last change, you could still have problems with 
a LogStream in the caller. (?)  Otherwise, why this:
https://bugs.openjdk.java.net/browse/JDK-8244999

This shouldn't be this hard :(

Coleen
>
>
>     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, Thomas