Bug 123999 – Move the setting up of callee's callFrame from pushFrame to callToJavaScript thunk

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

When on the C stack, we can't have a called C function set up the callee's stack since that space will be in use.  Instead, the callToJavaScript entry thunk should set up the frame.

[Michael Saboff]

Created attachment 218244 [details]
Patch that works for X86-64

Need to make changes in LowLevelInterpreter32_64.asm

[Michael Saboff]

Created attachment 218266 [details]
Patch with X86 32bit + 64bit changes tested.

Coded up changes for all but Win 32 and Win 64.  Tested Mac OSX 32 & 64.

Working on Win 32 and 64.

[Filip Pizlo]

Comment on attachment 218266 [details]
Patch with X86 32bit + 64bit changes tested.

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

LGTM so far, but revert some stuff and move some stuff into separate files.

> Source/JavaScriptCore/interpreter/CallFrame.cpp:154
> +void ProtoCallFrame::init(CodeBlock* codeBlock, JSScope* scope, JSObject* callee, JSValue thisValue, int argCountIncludingThis, JSValue* otherArgs)
> +{
> +    this->args = otherArgs;
> +    this->setCodeBlock(codeBlock);
> +    this->setScope(scope);
> +    this->setCallee(callee);
> +    this->setArgumentCountIncludingThis(argCountIncludingThis);
> +    size_t paddedArgsCount = argCountIncludingThis;
> +    if (codeBlock) {
> +        size_t numParameters = codeBlock->numParameters();
> +        if (paddedArgsCount < numParameters)
> +            paddedArgsCount = numParameters;
> +    }
> +    this->setPaddedArgsCount(paddedArgsCount);
> +    this->clearCurrentVPC();
> +    this->setThisValue(thisValue);
> +}
> +

Separate file, if you can put the declaration in a separate file also.

> Source/JavaScriptCore/interpreter/CallFrame.h:391
> +    struct ProtoCallFrame {
> +        Register codeBlockValue;
> +        Register scopeChainValue;
> +        Register calleeValue;
> +        Register argCountAndCodeOriginValue;
> +        Register thisArg;
> +        size_t paddedArgCount;
> +        JSValue *args;
> +
> +        void init(CodeBlock*, JSScope*, JSObject*, JSValue, int, JSValue* otherArgs = 0);
> +        CodeBlock* codeBlock() const { return codeBlockValue.Register::codeBlock(); }
> +        void setCodeBlock(CodeBlock* codeBlock) { codeBlockValue = codeBlock; }
> +        void setScope(JSScope* scope) { scopeChainValue = scope; }
> +        void setCallee(JSObject* callee) { calleeValue = Register::withCallee(callee); }
> +        int argumentCountIncludingThis() const { return argCountAndCodeOriginValue.payload(); }
> +        int argumentCount() const { return argumentCountIncludingThis() - 1; }
> +        void setArgumentCountIncludingThis(int count) { argCountAndCodeOriginValue.payload() = count; }
> +        void setPaddedArgsCount(size_t argCount) { paddedArgCount = argCount; }
> +
> +        void clearCurrentVPC() { argCountAndCodeOriginValue.tag() = 0; }
> +        void setThisValue(JSValue value) { thisArg = value; }
> +        void setArgument(size_t argument, JSValue value)
> +        {
> +            ASSERT(static_cast<int>(argument) < argumentCount());
> +            args[argument] = value;
> +        }
> +        static int offsetForArgumentCount()
> +        {
> +            return OBJECT_OFFSETOF(ProtoCallFrame, argCountAndCodeOriginValue) + OBJECT_OFFSETOF(EncodedValueDescriptor, asBits.payload);
> +        }
> +
> +    };

Please move this to a separate file if possible.

> Source/WTF/wtf/DataLog.cpp:43
> -#define DATA_LOG_TO_FILE 0
> +#define DATA_LOG_TO_FILE 1

Revert!

> Source/WTF/wtf/Platform.h:774
> -#define ENABLE_CONCURRENT_JIT 1
> +#define ENABLE_CONCURRENT_JIT 0
>  #endif

Why can't you just do --enableConcurrentJIT=false?

[Michael Saboff]

Created attachment 218383 [details]
Patch

Tested on Mac 32 bit & 64 bit, ARMv7, ARM64 & Windows 32 bit.  Also tested using C-Loop interpreter on Mac.  Builds on Windows 64 bit.

[Build Bot]

Comment on attachment 218383 [details]
Patch

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

[Build Bot]

Comment on attachment 218383 [details]
Patch

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

[Build Bot]

Comment on attachment 218383 [details]
Patch

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

[Michael Saboff]

Created attachment 218404 [details]
Patch for landing

Fixed WebKit-2 build issue.

[Michael Saboff]

Committed r160094: <http://trac.webkit.org/changeset/160094>