Bug 39901 – Simplified the host calling convention

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

[Geoffrey Garen]

I'm posting three patches:

- A behavior change patch that includes the interesting bits.
- A global find-and-replace patch that makes everything compatible with the new calling convention.
- A megapatch for the EWS bot to test, not for actual review.

[Geoffrey Garen]

Created attachment 57378 [details]
megapatch - DO NOT REVIEW

[Geoffrey Garen]

Created attachment 57379 [details]
PART ONE: Functional code changes

[Geoffrey Garen]

Created attachment 57380 [details]
PART TWO: Global search and replace

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSClipboardCustom.cpp:70:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 111 files


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

[Geoffrey Garen]

Created attachment 57381 [details]
PART TWO: Global search and replace

Fixed style bot issue.

[Eric Seidel (no email)]

Attachment 57381 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2624046

[Early Warning System Bot]

Attachment 57381 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2602059

[Sam Weinig]

Comment on attachment 57379 [details]
PART ONE: Functional code changes

JavaScriptCore/jit/JITOpcodes32_64.cpp:306
 +      subPtr(Imm32(16 - sizeof(void*)), stackPointerRegister);
These stack alignment operations could use a comment.

JavaScriptCore/jit/JITOpcodes.cpp:325
 +      };
These empty structs weird me out a bit.

[Gavin Barraclough]

Comment on attachment 57379 [details]
PART ONE: Functional code changes

JavaScriptCore/interpreter/Interpreter.cpp:632
 +      m_registerFile.shrink(callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters);
Is this a functional change we should land in a separate patch, in case it has unexpected side-effects?

JavaScriptCore/interpreter/Interpreter.cpp:710
 +      ASSERT(!callFrame->globalData().exception);
Could be ASSERT(!callFrame->hadException());

JavaScriptCore/interpreter/Register.h:61
 +          Register& operator=(JSObject*);
This is a world of danger.  Please to be making this a static function (e.g. make people do something explicit like "Register::withFunction()"), or only write functions to the register file as JSValues.

JavaScriptCore/jit/JITOpcodes32_64.cpp:281
 +      emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0);
I think you wanted to change both T1s to T0s here.

JavaScriptCore/jit/JITOpcodes32_64.cpp:399
 +      emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0);
another T1 -> T0 required

[Geoffrey Garen]

(In reply to comment #8)
> (From update of attachment 57379 [details])
> JavaScriptCore/jit/JITOpcodes32_64.cpp:306
>  +      subPtr(Imm32(16 - sizeof(void*)), stackPointerRegister);
> These stack alignment operations could use a comment.

Fixed.

> JavaScriptCore/jit/JITOpcodes.cpp:325
>  +      };
> These empty structs weird me out a bit.

Got rid of them.

[Geoffrey Garen]

(In reply to comment #9)
> (From update of attachment 57379 [details])
> JavaScriptCore/interpreter/Interpreter.cpp:632
>  +      m_registerFile.shrink(callFrame->registers() + callFrame->codeBlock()->m_numCalleeRegisters);
> Is this a functional change we should land in a separate patch, in case it has unexpected side-effects?

Maybe. I chose to include it because it helped solve some of the stack overflow layout test changes. The alternative would be to change the expected results on even more tests, which I was trying to avoid.

> JavaScriptCore/interpreter/Interpreter.cpp:710
>  +      ASSERT(!callFrame->globalData().exception);
> Could be ASSERT(!callFrame->hadException());

Fixed.

> 
> JavaScriptCore/interpreter/Register.h:61
>  +          Register& operator=(JSObject*);
> This is a world of danger.  Please to be making this a static function (e.g. make people do something explicit like "Register::withFunction()"), or only write functions to the register file as JSValues.

Fixed.

> 
> JavaScriptCore/jit/JITOpcodes32_64.cpp:281
>  +      emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0);
> I think you wanted to change both T1s to T0s here.
> 
> JavaScriptCore/jit/JITOpcodes32_64.cpp:399
>  +      emitGetFromCallFrameHeaderPtr(RegisterFile::ScopeChain, regT1, regT0);
> another T1 -> T0 required

This API is a little weird, but it specifies that we're trying to fetch into T1, relative to T0. We want to preserve T0 for the sake of this following instruction:

    move(regT0, callFrameRegister); // Eagerly restore caller frame register to avoid loading from stack.

[Gavin Barraclough]

Comment on attachment 57379 [details]
PART ONE: Functional code changes

olliej says yay too, so r+

[Geoffrey Garen]

Everything's landed now, but I had to disabled ENABLE_JIT_OPTIMIZE_NATIVE_CALL on Windows for now, since it's mysteriously crashing on every test.

[Oliver Hunt]

Comment on attachment 57378 [details]
megapatch - DO NOT REVIEW

Clearing the review flag as this isn't for review...