RFR: 8343756: CAN_SHOW_REGISTERS_ON_ASSERT for Windows [v2]

[Thomas Stuefe]

On Fri, 8 Nov 2024 21:48:25 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Greetings,
>> 
>> This enhancement adds register printouts on asserts or guarantees into the hs_err_pid.log error reporting files for Windows.
>> 
>> Additionally, it saves an accurate EXCEPTION_RECORD into the  hs_err_pid.mdmp file, also for asserts or guarantees, so the assertion context is now easily accessible in Windbg using .ecxr.
>> 
>> For more details on this enhancement, please see the JIRA issue.
>> 
>> Testing: Tier 1,2,3, jdk_jfr
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one additional commit since the last revision:
> 
>   initialize report page

I think this is mostly fine. I did look over the Windows implementation, but did not check it (I assume you did). Did you check for both architectures?

One problem we have on Posix platforms is that ucontext_t was not meant to be copied around. IIRC, on some platforms (eg PPC), it is mother structure with a bunch of daughter structures that need to be copied too. Hope Windows is simpler.

src/hotspot/share/utilities/debug.cpp line 192:

> 190: #ifdef CAN_SHOW_REGISTERS_ON_ASSERT
> 191:   if (os::current_thread_id() == g_asserting_thread) {
> 192:     context = os::get_saved_assert_context(&siginfo);

I would leave the `g_assertion_context != nullptr` in. It should not happen, but its defensive and a bug at this point is difficult to figure out (because you just don't get a hs-err file)

src/hotspot/share/utilities/debug.hpp line 41:

> 39: #define CAN_SHOW_REGISTERS_ON_ASSERT
> 40: extern char* g_assert_poison;
> 41: extern const char* g_assert_poison_page_for_reporting;

This g_assert_poison_page_for_reporting only exists to recognize assert poison crash when the assert poison itself is disarmed, right? So, during error reporting?

Makes sense but proposal for simplification:
- have two pointers initialized to the poison at arm time. E.g. "g_assert_poison" (the real one) and "g_assert_poison_copy"
- on disarm, switch g_assert_poison but leave the other unchanged
- when checking, always check against the copy - it never changes
- now you can unify the several code pieces that do `if ((si->si_signo == SIGSEGV || si->si_signo == SIGBUS) && si->si_addr == g_assert_poison_copy)` to something like `bool VMError::was_assertion_poison_crash(siginfo)`

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

PR Review: https://git.openjdk.org/jdk/pull/21994#pullrequestreview-2433304994
PR Review Comment: https://git.openjdk.org/jdk/pull/21994#discussion_r1840335300
PR Review Comment: https://git.openjdk.org/jdk/pull/21994#discussion_r1840352286