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

Changes requested by coleenp (Reviewer).

src/hotspot/share/utilities/vmError.cpp line 1718:

> 1716:   } else if (after_report_action == AfterReportAction::Die) {
> 1717: #ifdef _WINDOWS
> 1718:     if (UseOSErrorReporting) {

Since you already have to use UseOSErrorReporting in this code, I don't see the purpose of the parameter and ARA code.  If you add a 'return' here and have the other code snippet do a return, this should be all you need.
I'm not convinced that the DebugBreak here for windows is useful, since if you want to attach to a debugger, you'd use ShowMessageBoxOnError (based on my sample size of  1).  But moving it here away from the vmassert macro seems fine.  I tested that this works.
When implementing WER, they'll want a non-empty mdmp file but that can probably be done with call to the minidump facility which can be factored out of the os::abort() call.
My original interest is to get meaningful mdmp files showing the correct stack trace of the crashing thread.  This doesn't break or fix that.

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

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