Bug 123444 – Adjust CallFrameHeader’s ReturnPC and CallFrame locations to match the native ABI

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming soon.

[Mark Lam]

Created attachment 215385 [details]
patch 1.

First patch.  It builds.  It runs.  It passes tests.  But it probably need some ChangeLog comments.  Uploading it to get some feedback first.

[EFL EWS Bot]

Comment on attachment 215385 [details]
patch 1.

Attachment 215385 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/17148072

[EFL EWS Bot]

Comment on attachment 215385 [details]
patch 1.

Attachment 215385 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/17108099

[Build Bot]

Comment on attachment 215385 [details]
patch 1.

Attachment 215385 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17138083

[Build Bot]

Comment on attachment 215385 [details]
patch 1.

Attachment 215385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/17058089

[Geoffrey Garen]

Comment on attachment 215385 [details]
patch 1.

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.

[Mark Lam]

(In reply to comment #6)
> (From update of attachment 215385 [details])
> 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.

As discussed offline, since the pre-existing emitPutToCallFrameHeader() takes a slot index and will not work for our purpose here, we'll stick with these CallerFrame and ReturnPC specific getter/putters.


> > 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.

Will fix.


> > 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.

Actually no, it does work.  It makes use of assignment operators in Register treating CallFrame* and Instruction* differently.  But I agree that this is a hack and obviously confusing to the reader.  I will change the implementation to make the assignment explicit and do away with the assignment operator hack.
 
> > 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.

Ahh, the perils of retaining stale comments (without thinking it through).  Will remove.

> > 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.

OK, will fix.

> > 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.

Agreed.  Will fix (as indicated above).

[Mark Lam]

Created attachment 215555 [details]
patch 2: new and improved after addressing all feedback.

[WebKit Commit Bot]

Attachment 215555 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/CodeOrigin.h', u'Source/JavaScriptCore/dfg/DFGJITCompiler.cpp', u'Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp', u'Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp', u'Source/JavaScriptCore/ftl/FTLLink.cpp', u'Source/JavaScriptCore/interpreter/CallFrame.h', u'Source/JavaScriptCore/interpreter/JSStack.h', u'Source/JavaScriptCore/interpreter/Register.h', u'Source/JavaScriptCore/jit/AssemblyHelpers.h', u'Source/JavaScriptCore/jit/JIT.cpp', u'Source/JavaScriptCore/jit/JITCall.cpp', u'Source/JavaScriptCore/jit/JITCall32_64.cpp', u'Source/JavaScriptCore/jit/JITInlines.h', u'Source/JavaScriptCore/jit/JITOpcodes.cpp', u'Source/JavaScriptCore/jit/JITOpcodes32_64.cpp', u'Source/JavaScriptCore/jit/JITOperations.cpp', u'Source/JavaScriptCore/jit/SpecializedThunkJIT.h', u'Source/JavaScriptCore/jit/ThunkGenerators.cpp', u'Source/JavaScriptCore/llint/LLIntData.cpp', u'Source/JavaScriptCore/llint/LowLevelInterpreter.asm']" exit_code: 1
Source/JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 215555 [details]
patch 2: new and improved after addressing all feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=215555&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +	- The Register type no longer supports CallFrame* and Instruction*.
> 
> Line contains tab character.  [whitespace/tab] [5]

I thought I'd fix that.  Will fix before landing.

[Geoffrey Garen]

Comment on attachment 215555 [details]
patch 2: new and improved after addressing all feedback.

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?

[Mark Lam]

(In reply to comment #11)
> (From update of attachment 215555 [details])
> 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.

Will fix, and replace uses of Invalid with 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.

This is not a bug, but is probably not expressed that well.  Hence the confusion.  The CallFrameHeaderSize is indeed 6 on 64-bit and 5 on 32-bit.  The CallFrameHeader does not include ThisArgument and FirstArgument, but Michael added those in his recent patch for convenience of accessing those slots.

Will rewrite as follows:

        enum CallFrameHeaderEntry {
            CodeBlock = sizeof(CallerFrameAndPC) / sizeof(Register),
            ScopeChain,
            Callee,
            ArgumentCount,
            CallFrameHeaderSize,

            // The following entries are not part of the CallFrameHeader but are provided here as a convenience:
            ThisArgument = CallFrameHeaderSize,
            FirstArgument,
        };

> (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?

I talked with Michael and he agreed that if we structure it this way, there's no need to explicitly spell out the values of those enums.  So, I'll skip this part.

[Mark Lam]

Thanks for the reviews.  Landed in r158315: <http://trac.webkit.org/r158315>.