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

[Kim Barrett]

> 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:
  - all: https://git.openjdk.org/jdk/pull/12651/files
  - new: https://git.openjdk.org/jdk/pull/12651/files/8efb9e4e..07251af2

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=12651&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12651&range=00-01

  Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/12651.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/12651/head:pull/12651

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