Bug 123999 - Move the setting up of callee's callFrame from pushFrame to callToJavaScript thunk
Summary: Move the setting up of callee's callFrame from pushFrame to callToJavaScript ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 116888
  Show dependency treegraph
 
Reported: 2013-11-07 10:10 PST by Michael Saboff
Modified: 2013-12-04 08:37 PST (History)
7 users (show)

See Also:


Attachments
Patch that works for X86-64 (33.05 KB, patch)
2013-12-02 17:32 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch with X86 32bit + 64bit changes tested. (38.95 KB, patch)
2013-12-02 22:20 PST, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch (65.70 KB, patch)
2013-12-03 22:10 PST, Michael Saboff
fpizlo: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (65.84 KB, patch)
2013-12-04 06:15 PST, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2013-11-07 10:10:56 PST
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.
Comment 1 Michael Saboff 2013-12-02 17:32:14 PST
Created attachment 218244 [details]
Patch that works for X86-64

Need to make changes in LowLevelInterpreter32_64.asm
Comment 2 Michael Saboff 2013-12-02 22:20:55 PST
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.
Comment 3 Filip Pizlo 2013-12-03 10:19:04 PST
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?
Comment 4 Michael Saboff 2013-12-03 22:10:59 PST
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.
Comment 5 Build Bot 2013-12-03 22:48:11 PST
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
Comment 6 Build Bot 2013-12-03 23:37:03 PST
Comment on attachment 218383 [details]
Patch

Attachment 218383 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42948162
Comment 7 Build Bot 2013-12-04 00:21:00 PST
Comment on attachment 218383 [details]
Patch

Attachment 218383 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42828106
Comment 8 Michael Saboff 2013-12-04 06:15:33 PST
Created attachment 218404 [details]
Patch for landing

Fixed WebKit-2 build issue.
Comment 9 Michael Saboff 2013-12-04 08:37:43 PST
Committed r160094: <http://trac.webkit.org/changeset/160094>