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

[Thomas Stüfe]

Hi,

On Mon, Jun 1, 2020 at 3: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
>
> 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



> 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