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

[David Holmes]

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.

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

Thanks,
David

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