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

[David Holmes]

On Wed, 11 Dec 2024 05:41:01 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> 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
>
> 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 :)

I never noticed the triple :)

> 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

I figured the only class that should know that much about how to walk frames should the frame class. In addition it would look strange for either of the other classes to export that kind of API for other classes to use.

Thanks for re-reviewing. Of course it needs another review now I fixed the indentation issue.

> src/hotspot/share/utilities/debug.cpp line 653:
> 
>> 651:   NativeStackPrinter nsp(Thread::current_or_null());
>> 652:   nsp.print_stack_from_frame(tty, fr, buf, sizeof(buf),
>> 653:                   false /* print_source_info */, -1 /* max stack */);
> 
> Should this be aligned with tty or is the current formatting preferred? Alternatively it seems like this could all fit on one line, but it's up to you

Fixed - thanks.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22472#discussion_r1879383236
PR Review Comment: https://git.openjdk.org/jdk/pull/22472#discussion_r1879382577
PR Review Comment: https://git.openjdk.org/jdk/pull/22472#discussion_r1879381123