[webkit-reviews] review denied: [Bug 123444]=?UTF-8?Q?=20Adjust=20CallFrameHeader=E2=80=99s=20ReturnPC=20and=20CallFrame=20locations=20to=20match=20the=20native=20ABI=20?=: [Attachment 215385] patch 1.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied 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 215385: patch 1.
https://bugs.webkit.org/attachment.cgi?id=215385&action=review

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


Looks like this needs another go-round.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:103
> -    emitPutToCallFrameHeader(GPRInfo::regT2, JSStack::ReturnPC);
> +    emitPutReturnPCToCallFrameHeader(GPRInfo::regT2);

Instead of creating an extra layer of helper functions, I think you should use
the existing emitPutToCallFrameHeader, and pass it returnPCOffset() as the
offset.

> Source/JavaScriptCore/dfg/DFGJITCompiler.cpp:131
> -	   emitGetFromCallFrameHeaderPtr(JSStack::CallerFrame,
GPRInfo::argumentGPR0);
> +	   emitGetCallerFrameFromCallFrameHeaderPtr(GPRInfo::argumentGPR0);

Ditto.

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:131
> -	   jit.store32(AssemblyHelpers::TrustedImm32(JSValue::CellTag),
AssemblyHelpers::tagFor((VirtualRegister)(inlineCallFrame->stackOffset +
JSStack::CallerFrame)));
> -	   jit.storePtr(callerFrameGPR,
AssemblyHelpers::payloadFor((VirtualRegister)(inlineCallFrame->stackOffset +
JSStack::CallerFrame)));
> -	   jit.storePtr(AssemblyHelpers::TrustedImmPtr(jumpTarget),
AssemblyHelpers::payloadFor((VirtualRegister)(inlineCallFrame->stackOffset +
JSStack::ReturnPC)));
> +	   jit.storePtr(callerFrameGPR,
AssemblyHelpers::Address(GPRInfo::callFrameRegister,
inlineCallFrame->callerFrameOffset()));
> +	   jit.storePtr(AssemblyHelpers::TrustedImmPtr(jumpTarget),
AssemblyHelpers::Address(GPRInfo::callFrameRegister,
inlineCallFrame->returnPCOffset()));

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:665
> +    m_jit.storePtr(GPRInfo::callFrameRegister,
calleeFrameCallerFrame(numArgs));

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3119
> -	   m_jit.emitGetFromCallFrameHeaderPtr(JSStack::ReturnPC,
GPRInfo::regT2);
> +	   m_jit.emitGetReturnPCFromCallFrameHeaderPtr(GPRInfo::regT2);
>	   // Restore our caller's "r".
> -	   m_jit.emitGetFromCallFrameHeaderPtr(JSStack::CallerFrame,
GPRInfo::callFrameRegister);
> +	  
m_jit.emitGetCallerFrameFromCallFrameHeaderPtr(GPRInfo::callFrameRegister);

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:744
> +    MacroAssembler::Address calleeFrameSlot(int numArgs, int slot, int
byteOffset = 0)

Instead of passing a byte offset to this function, callers should call this
function and then call .withOffset() on its return value. That seems to be the
existing idiom. It's strange to pass a byte offset to a function named
"calleeFrameSlot" because "slot" implies that we're counting JSValue-sized
stack slots.

> Source/JavaScriptCore/interpreter/CallFrame.h:132
> +	   void clearReturnPC() { registers()[JSStack::CallerFrameAndReturnPC]
= static_cast<Instruction*>(0); }

This is broken. You need to assign 0 to .vPC() in the union.

> Source/JavaScriptCore/interpreter/CallFrame.h:220
> +	   void setCallerFrame(CallFrame* callerFrame) {
static_cast<Register*>(this)[JSStack::CallerFrameAndReturnPC] = callerFrame; }

Ditto. You need to assign callerFrame to .callerFrame in the union.

> Source/JavaScriptCore/interpreter/CallFrame.h:299
> +	   void setReturnPC(void* value) {
static_cast<Register*>(this)[JSStack::CallerFrameAndReturnPC] =
(Instruction*)value; }

Ditto.

> Source/JavaScriptCore/interpreter/JSStack.h:59
> +	       // This is either an Instruction* or a pointer into JIT
generated code stored as an Instruction*.

Really? CallerFrameAndReturnPC is an Instruction*? I don't think it is.

> Source/JavaScriptCore/interpreter/Register.h:66
> +	   CallFrame* callerFrame() const;

This should be callFrame.

> Source/JavaScriptCore/interpreter/Register.h:103
> +	       CallFrame* callerFrame;

This should be named "callFrame", since our union members name their types, and
not what they're used for.

> Source/JavaScriptCore/interpreter/Register.h:109
> +	       } callerFrameAndPC;

Ditto.

> Source/JavaScriptCore/interpreter/Register.h:145
> +    ALWAYS_INLINE Register& Register::operator=(CallFrame* callerFrame)

This is a bad abstraction. Register is just a union. You're building into it
the layout of the stack. There's no guarantee that every time I store a
CallFrame* to the stack I'm trying to change my return address. You should
remove this assignment operator altogether.

> Source/JavaScriptCore/interpreter/Register.h:161
>      ALWAYS_INLINE Register& Register::operator=(Instruction* vPC)

Ditto.

> Source/JavaScriptCore/interpreter/Register.h:190
>      ALWAYS_INLINE Instruction* Register::vPC() const

Ditto.