[webkit-reviews] review granted: [Bug 123444]=?UTF-8?Q?=20Adjust=20CallFrameHeader=E2=80=99s=20ReturnPC=20and=20CallFrame=20locations=20to=20match=20the=20native=20ABI=20?=: [Attachment 215555] patch 2: new and improved after addressing all feedback.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 123444: Adjust CallFrameHeader’s ReturnPC and CallFrame locations to match
the native ABI
https://bugs.webkit.org/show_bug.cgi?id=123444

Attachment 215555: patch 2: new and improved after addressing all feedback.
https://bugs.webkit.org/attachment.cgi?id=215555&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215555&action=review


r=me

Please fix the enum.

> Source/JavaScriptCore/interpreter/JSStack.h:70
> +	       Invalid = 0,
> +	       CodeBlock = sizeof(CallerFrameAndPC) / sizeof(Register),
> +	       ScopeChain,
> +	       Callee,
> +	       ArgumentCount,
> +	       ThisArgument,
> +	       FirstArgument,
> +	       CallFrameHeaderSize = ThisArgument,

A few things here:

(1) Let's not add Invalid to this enum. It's curious to tempt someone into
using an invalid constant. Instead, let's change the code that wants an invalid
mapped virtual register number to use CodeBlock or INT_MAX.

(2) Is CallFrameHeaderSize really ThisArgument? On 64bit, ThisArgument is 6,
but there are 8 entries in the call frame header. This seems like a bug. Are
you trying to exclude CallerFrameAndPC from the header size? That seems weird.

(3) Michael mentioned that he liked the enum in the other order, so he could
see how things laid out on the stack. Can you add a comment that shows the
in-memory order of these values, or do something else to preserve that feature?