RFR: 8311542: Consolidate the native stack printing code [v5]

[Julian Waters]

On Tue, 10 Dec 2024 23:15:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> We now print native stacks in a number of contexts, not just VMError reporting, and we have to try `os::platform_print_native_stack` else fall back to `VMError::print_native stack`. 
>> 
>> The refactoring adds a new `NativeStackPrinter` (NSP) helper class which can be constructed with some of the context information for the printing that will follow. This avoids the need for the print functions to have a large number of parameters. We have to expose both the top-level printing functionality and the "lower-level" frame-based stack walk as the error reporter needs access to that directly. To maintain the exact same format of output the NSP has to be aware of some error reporter usage requirements.
>> 
>> I also had to expose a direct `frame` taking print function for the Debug.cpp `pns` case.
>> 
>> Testing:
>>  - tiers 1 - 4
>> 
>> Some frame management code was also moved from `VMError` to the `frame` class.
>
> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update copyright notice to reflect file where code was copied from

Looks good

src/hotspot/share/runtime/frame.cpp line 1564:

> 1562:  * Gets the caller frame of `fr` for thread `t`.
> 1563:  *
> 1564:  * @returns an invalid frame (i.e. fr.pc() === 0) if the caller cannot be obtained

I got a bit of a chuckle looking at this, the triple equals reminds me of the wonky JavaScript equality checking. Not a review, I just found it funny :)

src/hotspot/share/runtime/frame.hpp line 443:

> 441:   void print_on_error(outputStream* st, char* buf, int buflen, bool verbose = false) const;
> 442:   static void print_C_frame(outputStream* st, char* buf, int buflen, address pc);
> 443:   static frame next_frame(frame fr, Thread* t); // For native stack walking

A thought: Does it make more sense to have next_frame implemented in vmError or nativeStackPrinter and made available through their respective headers rather than as a member of the frame class? Just thinking out loud, I'm fine with it as is

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

Marked as reviewed by jwaters (Committer).

PR Review: https://git.openjdk.org/jdk/pull/22472#pullrequestreview-2494367141
PR Review Comment: https://git.openjdk.org/jdk/pull/22472#discussion_r1879377777
PR Review Comment: https://git.openjdk.org/jdk/pull/22472#discussion_r1879376735