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.
Created attachment 57378 [details] megapatch - DO NOT REVIEW
Created attachment 57379 [details] PART ONE: Functional code changes
Created attachment 57380 [details] PART TWO: Global search and replace
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.
Created attachment 57381 [details] PART TWO: Global search and replace Fixed style bot issue.
Attachment 57381 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/2624046
Attachment 57381 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2602059
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.
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
(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.
(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.
Comment on attachment 57379 [details] PART ONE: Functional code changes olliej says yay too, so r+
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.
Comment on attachment 57378 [details] megapatch - DO NOT REVIEW Clearing the review flag as this isn't for review...