RFR: 8302798: Refactor -XX:+UseOSErrorReporting for noreturn crash reporting [v2]

[Coleen Phillimore]

On Thu, 23 Feb 2023 09:20:43 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this change to the implementation of the Windows-specific option
>> UseOSErrorReporting, toward allowing crash reporting functions to be declared
>> noreturn.  VMError::report_and_die no longer conditionally returns if the
>> Windows-only option UseOSErrorReporting is true.
>> 
>> The core of VM crash reporting has been renamed and privatized, and now takes
>> an additional argument indicating whether the caller just wants reporting or
>> also wants termination after reporting.  The Windows top-level and unhandled
>> exception filters call into that to request reporting, and also termination if
>> UseOSErrorReporting is false, otherwise returning from the call and taking
>> further action.  All other callers of VM crash reporting include termination.
>> 
>> So if we get a structured exception on Windows and the option is true then we
>> report it and then end up walking up the exception handlers until running out,
>> when Windows crash behavior gets a shot.  This is essentially the same as
>> before the change, just checking the option in a different place (in the
>> Windows-specific caller, rather than in an #ifdef block in the shared code).
>> 
>> In addition, the core VM crash reporter uses BREAKPOINT before termination if 
>> UseOSErrorReporting is true.  So if we have an assertion failure we report it
>> and then end up walking up the exception handlers (because of the breakpoint
>> exception) until running out, when Windows crash behavior gets a shot.
>> 
>> So if there is an attached debugger (or JIT debugging is enabled), this leads
>> an assertion failure to hit the debugger inside the crash reporter rather than
>> after it returned (due to the old behavior for UseOSErrorReporting) from the
>> failed assertion (hitting the BREAKPOINT there).
>> 
>> Note that on Windows, ::breakpoint() calls DebugBreak(), raising a breakpoint
>> exception, which ends up being unhandled if UseOSErrorReporting is true.
>> 
>> There is a pre-existing bug that I'll be reporting separately.  If
>> UseOSErrorReporting and CreateCoredumpOnCrash are both true, we create an
>> empty .mdmp file.  We shouldn't create that file when UseOSErrorReporting.
>> 
>> Testing:
>> mach5 tier1-3
>> 
>> Manual testing with the following, to verify expected behavior that is the
>> same as before the change.
>> 
>> # -XX:ErrorHandlerTest=N
>> # 1: assertion failure
>> # 2: guarantee failure
>> # 14: SIGSEGV
>> # 15: divide by zero
>> path/to/bin/java \
>>   -XX:+UnlockDiagnosticVMOptions \
>>   -XX:+ErrorLogSecondaryErrorDetails \
>>   -XX:+UseOSErrorReporting \
>>   -XX:ErrorHandlerTest=1 \
>>   TestDebug.java
>> 
>> --- TestDebug.java ---
>> import java.lang.String;
>> public class TestDebug {
>>     static private volatile String dummy;
>>     public static void main(String[] args) throws Exception {
>>         while (true) {
>>             dummy = new String("foo bar");
>>         }
>>     }
>> }
>> --- end TestDebug.java ---
>> 
>> I need someone with a better understanding of Windows to test the interaction
>> with Windows Error Reporting (WER).
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   check after_report_action

src/hotspot/os/windows/vmError_windows.cpp line 39:

> 37:                                 exceptionInfo->ExceptionRecord,
> 38:                                 exceptionInfo->ContextRecord,
> 39:                                 "%s", "");

Or if you want to make report_and_die marked noreturn, refactor it without the ARA stuff so report_and_die() is no-return, report_and_maybe_die() is return and both call the impl that uses UseOSErrorReporting.

-------------

PR: https://git.openjdk.org/jdk/pull/12651