RFR: 8314508: Improve how relativized pointers are printed by frame::describe [v2]

[Coleen Phillimore]

On Fri, 8 Mar 2024 14:15:03 GMT, Fredrik Bredberg <fbredberg at openjdk.org> wrote:

>> src/hotspot/share/runtime/frame.cpp line 1634:
>> 
>>> 1632:       st->print_cr(" %s  %s %s", spacer, spacer, fv.description);
>>> 1633:     } else {
>>> 1634:       if (*fv.description == '#' && isdigit(fv.description[1])) {
>> 
>> Is it possible for fv.description to be null?  its sort of unclear from the context. There are several pointer dereferences here.
>> 
>> Can you add a comment about what the format of this is?  What is fv.description supposed to be? #<what number?>
>> 
>> And fp.location is the relativized offset, if fp.description is NOT #<number>?
>> 
>> This really does need some comment.  And maybe a local variable to say what *fv.location is.
>> 
>> And after testing the #<some number> output, the code still has to see if fv.location is between -100 and 100?  Should it not just check fp != nullptr ?
>
> @coleenp 
> 
> I'll try to answer as good as I can.
> 
> **Q1:** Is it possible for `fv.description` to be null? its sort of unclear from the context. There are several pointer dereferences here.
> **A1:** If `fv.description` ever had been null (before any of my changes) it would have lead to a crash, so in my world it is never null.
> 
> **Q2:** Can you add a comment about what the format of this is? What is `fv.description` supposed to be? #<what number?>
> **A2:** If you look at the stack frame in the description of this PR you'll see the contents of the `fv.description` after the `<address>: <content>` columns.
>     The only `fv.description` string starting with a '#' is the line for the saved frame pointer. So, `#10 method java.lang.invoke.LambdaForm...` basicaly means frame 10.
>     This is also how I find the frame pointer.
> 
> **Q3:** This really does need some comment. And maybe a local variable to say what `*fv.location` is.
> **A3:** `fv.location` contains the `<address>:` columns value and `*fv.location` contains the `<content>` columns value.
> 
> However if the value at an address is relativized like 0x000000409e14cc20 in the example, the real content of address 0x000000409e14cc20 is 0x0000000000000003. So instead of showing the contents as 3 (which we used to do before my patch) we show the real derelativized pointer value which is 0x000000409e14cc88 and also add the text `(relativized: fp+3)` to try to explain to anyone that's confused by the fact that the real content of the stack address is 3.
> 
> **Q4:** And after testing the # output, the code still has to see if fv.location is between -100 and 100? Should it not just check `fp != nullptr` ?
> **A4:** No. In order for us to believe that `*fv.location` is a fp-relative value it must be a reasonable small positive/negative number. Otherwise we would print all large pointer values as fp-relative.
> 
> The oddball situation is that I've found frame pointer addresses pointing to a content which is within -100 and 100, with a `fv.description` string starting with a '#'. And those a not relativized values, and therefore should not be printed as such. Hence the `*fv.description != '#'` condition.
> 
> Hope this shed some light on this code.

Ok, your answers make sense studying the code and the output together.  None of these values are null and I don't think adding a null check is helpful now.

[6.693s][trace][continuations]  0x000000409e14cc50: 0x000000409e14cc00 interpreter_frame_last_sp        (relativized: fp-14)

Maybe adding a comment before this line like:
     // The fv.description string starting with a '#' is the line for the saved frame pointer eg.
     // So, #10 method java.lang.invoke.LambdaForm... basicaly means frame 10.

Then say later before line 1638, what you said in this comment.
    //  Some frame pointer addresses pointing to a content which is within -100 and 100, with a fv.description
    // string starting with a '#'. And those a not relativized values, and therefore should not be printed as such.

Then reading the code again, you'll or someone else will have an idea of why.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18102#discussion_r1520297192