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

[Thomas Stüfe]

On Mon, Jun 1, 2020 at 6:21 PM <coleen.phillimore at oracle.com> wrote:

>
>
> 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> 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
>
>
To me it is not clear if this is a proactive cleanup or driven by an actual
bug. But IIRC we removed all usages of ResourceArea from outputStream child
classes, so usage of ResourceMark should not be influenced by outputStream.
It may have bitrotted, then we should fix it. Or it may just not be common
knowledge since the patches came from SAP.


> This shouldn't be this hard :(
>
>
I agree.

..Thomas


> 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
>
>
>
>