RFR: 8296469: Instrument VMError::report with reentrant iteration step for register and stack printing [v11]

[Axel Boldt-Christmas]

On Mon, 15 May 2023 10:27:26 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Rename and invert should_stop_reattempt_step
>
> src/hotspot/os_cpu/aix_ppc/os_aix_ppc.cpp line 480:
> 
>> 478:   int n = continuation;
>> 479:   if (context == nullptr || n < 0 || n >= register_count) {
>> 480:     return;
> 
> Here and the other variants: Under which circumstances would n<0 be acceptable? I would assert. Arguably also for context=nullptr, but up to you.

Done.

Constrained the continuation value to be in the range `[0, register_count]`

> src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp line 385:
> 
>> 383:     // Update continuation with next index before printing location
>> 384:     continuation = n + 1;
>> 385:     st->print("%-*.*s=", 8, 8, reg_abi_names[n]);
> 
> Nitpicking, preexisting: while you are here, you could probably simplify to "%-8.8s"

Done.

> src/hotspot/os_cpu/linux_s390/os_linux_s390.cpp line 478:
> 
>> 476:     } else {
>> 477:       st->print("r%-2d=", n-1);
>> 478:       print_location(st, uc->uc_mcontext.gregs[n-1]);
> 
> The "-1" here and the "-3" for ppc makes me a bit nervous, since if this bitrots, we probably never notice that we print the wrong registers unless someone debugs. Pragmatic proposal: Print the general regs first, then the special ones, then you don't need to deal with hard-coded offsets. I don't think anyone would object to that.

Done.

Not entirely sure how this would rot and not be noticeable. (At least less noticeable than with no hard coded index offset)

> src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.cpp line 239:
> 
>> 237:     continuation = n + 1;
>> 238: # define CASE_PRINT_REG(n, str, id) case n: st->print(str); print_location(st, uc->id);
>> 239:     switch (n) {
> 
> here and other places: if you wanted, you could get rid of the first argument to CASE_PRINT_REG by using a second int variable running alongside n.
> 
> Up to you though, this is also fine as it is.

Unsure why I wrote them all like this. But think I will leave it.

> src/hotspot/share/utilities/vmError.cpp line 510:
> 
>> 508:       st->print("stack at sp + %d slots: ", i);
>> 509:       os::print_location(st, *(slot));
>> 510:     }
> 
> Can you please print a note for unreadable slots too?

Done.

Added early exit if sp is misaligned

> src/hotspot/share/utilities/vmError.cpp line 637:
> 
>> 635:     _current_step = __LINE__;                              \
>> 636:     _current_step_info = s;                                \
>> 637:     record_step_start_time();                              \
> 
> Preexisting: mover record_step_start_time and _step_did_timeout into condition?

Done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194880581
PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194878547
PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194878444
PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194877190
PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194876501
PR Review Comment: https://git.openjdk.org/jdk/pull/11017#discussion_r1194875121