1508223 - Rename and document {JsJitFrameIter,WasmFrameIter}::returnAddressToFp

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Julian Seward [:jseward]]

Bug 1476251 introduces new functions
{JsJitFrameIter,WasmFrameIter}::returnAddressToFp.  Their names have caused
some confusion and in particular it isn't clear from the names what they
actually do.  Hence they should be renamed and documented.

It may also be the case that we should take the opportunity to clean up some
of the naming associated with stack walking.  In particular I observed:

* the use of "up" and "down" or equivalents (above/below, on top/underneath)
  is ambiguous, because it depends on the author's assumption about the
  direction of stack growth (down or up?)  I find "caller" (the fn that called
  me) vs "callee" (the fn I am calling) to be non-ambiguous, as are "inner" vs
  "outer".

* there may be places where a code address is misleadingly (via naming or
  comments) stated to be, in effect "the return address of my immediate
  callee", but where in fact there is no callee, for example because the
  current frame is stopped at a trap or breakpoint.

  In such cases the referred-to instruction is more accurately "the next
  instruction in this frame that will execute" and should be renamed
  accordingly, perhaps "nextPC" or "nextInstructionAddress".

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for opening an issue. I don't think "nextPC" is correct; when we are unwinding, returnAddressToFp is the next instruction only when we're unwinding the current frame. It's not the case for the following frames.

Let's take an example to see what returnAddressToFp is; maybe it'll clear up our understanding, and will inspire us for names.

Say you're stopping execution of wasm (or jit, for what it's worth) code at a given point and want to get a stack (say, because we reach a trap in wasm, or a JS callee makes a stack with new Error().stack). Imagine we have the following frame layout:

--------------- <- callerFP
| wasm caller |
--------------- <- calleeFP
| wasm callee |
--------------- <- exitFP
| exit frame  |
---------------

The frame iterator starts with FP := exitFP in the general case, and pops the exit frame. The return-address-to-fp is the return address in calleeFP, that is the instruction that executes after the call to the exit frame returned. So it's the return address into the frame that's currently described, which probably was the reason behind the name "returnAddressToFp". After unwinding this frame, FP is set to calleeFP. Then we can describe the first wasm frame (wasm callee).

When we unwind to the next frame (wasm caller), the return address is set to the PC value in the wasm caller right after the call instruction into wasm callee, and that's the new value of returnAddressToFP. FP is set to callerFP.

"nextInstruction" feels not complete, because it doesn't tell in which frame (candidates can be in the current frame or in the caller's frame). Maybe we could make it very explicit, like nextInstructionInCurrentFrame, or returnAddressToCurrentFrame?

Other than that, I concur that innermost/outermost and down/up are misleading when talking about stack layout, if not commonly agreed.

Comment 2

[Julian Seward [:jseward]]

Benjamin and I discussed this on irc a bit.

It appears that the meaning of |aFrame.returnAddressToFp()| is to return the
address of the next instruction to execute in |aFrame| once control returns to
that frame.

This is also consistent with how the stackmaps patch (bug 1476251) uses it.

That leaves the question of what a good name for this function would be.

When we temporarily freeze a thread (for whatever reason) and iterate over its
frames, all but the innermost frame will eventually regain control when their
immediate callees return.  But the innermost frame itself must have stopped
for some other reason: a trap or a debug break.  For such a frame, the
next-address-to-execute is not the return address of its callee, because there
is no callee -- it is not calling anything.  So it would be most accurate for
the chosen name not to mention or imply that "this is a return address".

Comment 3

[Lars T Hansen [:lth]]

Good discussion.

Having thought about it, I think the name "returnAddressToFP" grated on my ears for several reasons: "To" is the wrong preposition, though "For" (or "In") might have worked; "FP" is less appropriate than "Frame"; and getting both "returnAddress" and "FP" in the same word - both of them describing addresses that really are not all that directly related - is very confusing.  

Names like "nextInstruction" and "nextPC" are too generic to be helpful, I think.

I like "nextInstructionInCurrentFrame", as Benjamin suggested, but "next" is pretty vague still and "instruction" is not quite right either.  "resumePointInCurrentFrame" (or just resumePointInFrame) is more specific and avoids the problematic "returnAddress", except "resumePoint" is easily confused with the OSR stuff.  "resumeAddressInCurrentFrame" might work.  ("resumptionAddressInCurrentFrame" might be more grammatically correct, but is a real handful to type.)

Comment 4

[Julian Seward [:jseward]]

How about "resumePCinCurrentFrame"?  That removes the uncertainty,
for the casual reader, of whether "Point"/"Address" refers to a code
or a stack address.

That said I could live with "resumeAddressInCurrentFrame".

Comment 5

[Lars T Hansen [:lth]]

(In reply to Julian Seward [:jseward] from comment #4)

> How about "resumePCinCurrentFrame"?  

I could live with that too.

Comment 6

[Julian Seward [:jseward]]

Attachment: bug1508223-rename-returnAddressToFp-2.diff

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1508223-rename-returnAddressToFp-3.diff

Comment 8

[Lars T Hansen [:lth]]

Comment on attachment 9031840 [details] [diff] [review]
bug1508223-rename-returnAddressToFp-3.diff

Review of attachment 9031840 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.  Good comment, too.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8cd60eb63f
Rename and document {JsJitFrameIter,WasmFrameIter}::returnAddressToFp.  r=lth.

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/cf8cd60eb63f