RFR: 8266404: Fatal error report generated with -XX:+CrashOnOutOfMemoryError should not contain suggestion to submit a bug report

[Thomas Stüfe]

Hi David,

On Thu, May 13, 2021 at 10:04 AM David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> Thanks for looking at this.
>
> On 13/05/2021 3:51 pm, Thomas Stuefe wrote:
> > On Thu, 13 May 2021 02:14:43 GMT, David Holmes <dholmes at openjdk.org>
> wrote:
> >
> >> A simple fix to add a new VMErrorType (OOM_JAVA_HEAP_FATAL) to indicate
> we've requested a Java OOM error to be fatal. A new overload of
> report_fatal is provided to allow this to be passed through.
> >>
> >> VMError has an existing notion of "should_report_bug" but that
> encompasses more than just printing the bug submission URL, so I added a
> new specific check for that.
> >>
> >> Checked hs_err files before and after with the fix, from the
> CrashOnOutOfMemoryError test.
> >>
> >> Tested tiers 1-3 for good measure.
> >>
> >> Thanks,
> >> David
> >
> > Hi David,
> >
> > mostly fine, minor nits.
> >
> > Side note, I really would like to clean up this coding a bit. I have
> attempted to so so multiple times, but the patches ballooned. I may try
> again, maybe in tiny baby steps.
> >
> > Cheers, Thomas
> >
> > src/hotspot/share/utilities/debug.cpp line 294:
> >
> >> 292: void report_fatal_impl(VMErrorType error_type, const char* file,
> int line,
> >> 293:                        const char* detail_fmt, va_list detail_args)
> >> 294:   ATTRIBUTE_PRINTF(4,0);
> >
> > Can prototype and implementation be merged while retaining the attrib
> marker?
>
> Unfortunately no, that is why they are both present and commented.
>
> > src/hotspot/share/utilities/debug.cpp line 298:
> >
> >> 296: // Definition
> >> 297: void report_fatal_impl(VMErrorType error_type, const char* file,
> int line,
> >> 298:                        const char* detail_fmt, va_list
> detail_args) {
> >
> > Can this function be static? Any reason to expose it?
>
> No reason to expose. Changed to static.
>
> > src/hotspot/share/utilities/debug.hpp line 154:
> >
> >> 152:   OOM_MMAP_ERROR   = 0xe0000002,
> >> 153:   OOM_MPROTECT_ERROR = 0xe0000003,
> >> 154:   OOM_JAVA_HEAP_FATAL = 0xe000004
> >
> > Add one more zero? To keep it in line with the other errors?
>
> Ooops! Well spotted. I've no idea why these are expressed this way
> though and not just 0, 1, 2, 3, 4.
>
>
This enum, signal numbers (posix) and windows SEH codes are mashed together
in VMError::_id. I guess this is some effort to make sure value ranges for
these three semantically different things don't intersect.

I don't like this design though. I would like to keep these as two separate
things: at the highest lvl, differentiate between internal fatal errors and
crashes. For each type keep detail info: For crashes,
signal number+context+siginfo. For asserts, error type and maybe context.


> > src/hotspot/share/utilities/debug.hpp line 168:
> >
> >> 166:                             int status, const char* detail);
> >> 167: void report_fatal(const char* file, int line, const char*
> detail_fmt, ...) ATTRIBUTE_PRINTF(3, 4);
> >> 168: void report_fatal(VMErrorType error_type, const char* file, int
> line, const char* detail_fmt, ...) ATTRIBUTE_PRINTF(4, 5);
> >
> > See above, can we not expose this?
>
> ?? These are the ones that are meant to be exposed. We hide behind the
> fatal() macro for the first case but these are the exported API.
>
>
Yes, but atm the sole user of this api lives in debug.cpp. Otherwise this
API is only exposed via the specialized report_java_out_of_memory().

Never mind though, it is not that important.


> Thanks,
> David
>
>
Patch looks good to me.

Cheers, Thomas


> > -------------
> >
> > Changes requested by stuefe (Reviewer).
> >
> > PR: https://git.openjdk.java.net/jdk/pull/4006
> >
>