WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
39901
Simplified the host calling convention
https://bugs.webkit.org/show_bug.cgi?id=39901
Summary
Simplified the host calling convention
Geoffrey Garen
Reported
2010-05-28 14:25:13 PDT
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.
Attachments
megapatch - DO NOT REVIEW
(456.59 KB, patch)
2010-05-28 14:25 PDT
,
Geoffrey Garen
no flags
Details
Formatted Diff
Diff
PART ONE: Functional code changes
(98.31 KB, patch)
2010-05-28 14:26 PDT
,
Geoffrey Garen
barraclough
: review+
Details
Formatted Diff
Diff
PART TWO: Global search and replace
(375.41 KB, patch)
2010-05-28 14:26 PDT
,
Geoffrey Garen
no flags
Details
Formatted Diff
Diff
PART TWO: Global search and replace
(375.41 KB, patch)
2010-05-28 14:33 PDT
,
Geoffrey Garen
sam
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Geoffrey Garen
Comment 1
2010-05-28 14:25:51 PDT
Created
attachment 57378
[details]
megapatch - DO NOT REVIEW
Geoffrey Garen
Comment 2
2010-05-28 14:26:20 PDT
Created
attachment 57379
[details]
PART ONE: Functional code changes
Geoffrey Garen
Comment 3
2010-05-28 14:26:52 PDT
Created
attachment 57380
[details]
PART TWO: Global search and replace
WebKit Review Bot
Comment 4
2010-05-28 14:30:32 PDT
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
Comment 5
2010-05-28 14:33:54 PDT
Created
attachment 57381
[details]
PART TWO: Global search and replace Fixed style bot issue.
Eric Seidel (no email)
Comment 6
2010-05-28 14:53:20 PDT
Attachment 57381
[details]
did not build on mac: Build output:
http://webkit-commit-queue.appspot.com/results/2624046
Early Warning System Bot
Comment 7
2010-05-28 15:15:26 PDT
Attachment 57381
[details]
did not build on qt: Build output:
http://webkit-commit-queue.appspot.com/results/2602059
Sam Weinig
Comment 8
2010-05-28 15:30:05 PDT
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 9
2010-05-28 15:40:55 PDT
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
Comment 10
2010-05-28 17:08:44 PDT
(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
Comment 11
2010-05-28 17:11:18 PDT
(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 12
2010-05-28 17:34:55 PDT
Comment on
attachment 57379
[details]
PART ONE: Functional code changes olliej says yay too, so r+
Geoffrey Garen
Comment 13
2010-05-29 01:14:31 PDT
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 14
2010-07-02 12:33:46 PDT
Comment on
attachment 57378
[details]
megapatch - DO NOT REVIEW Clearing the review flag as this isn't for review...
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug