RESOLVED FIXED 148659
JavaScript functions should restore the stack pointer after a call 
https://bugs.webkit.org/show_bug.cgi?id=148659
Summary JavaScript functions should restore the stack pointer after a call
Basile Clement
Reported 2015-08-31 17:02:57 PDT
...
Attachments
Patch (7.61 KB, patch)
2015-08-31 17:04 PDT, Basile Clement 		no flags
DetailsFormatted DiffDiff
Patch (132.61 KB, patch)
2015-08-31 17:05 PDT, Basile Clement 		no flags
DetailsFormatted DiffDiff
Patch (7.61 KB, patch)
2015-08-31 17:05 PDT, Basile Clement 		no flags
DetailsFormatted DiffDiff
Patch (7.87 KB, patch)
2015-09-01 14:41 PDT, Basile Clement 		no flags
DetailsFormatted DiffDiff
Patch (8.99 KB, patch)
2015-09-02 16:25 PDT, Basile Clement 		msaboff: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Basile Clement
Comment 1 2015-08-31 17:04:41 PDT
Created attachment 260333 [details] Patch
Basile Clement
Comment 2 2015-08-31 17:05:19 PDT
Created attachment 260334 [details] Patch
Basile Clement
Comment 3 2015-08-31 17:05:48 PDT
Created attachment 260335 [details] Patch
Saam Barati
Comment 4 2015-09-01 07:48:47 PDT
Comment on attachment 260335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260335&action=review > Source/JavaScriptCore/ftl/FTLJSCall.cpp:59 > + jit.addPtr(CCallHelpers::TrustedImm32(sizeof(void*) - static_cast<int64_t>(stackSize)), CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister); Why the "-" here?
Saam Barati
Comment 5 2015-09-01 08:02:46 PDT
(In reply to comment #4) > Comment on attachment 260335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=260335&action=review > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:59 > > + jit.addPtr(CCallHelpers::TrustedImm32(sizeof(void*) - static_cast<int64_t>(stackSize)), CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister); > > Why the "-" here? I think what I'm more curious about is why are we not subtracting the entire stack size, but subtracting stack size minus 8 bytes
Basile Clement
Comment 6 2015-09-01 11:16:30 PDT
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 260335 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=260335&action=review > > > > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:59 > > > + jit.addPtr(CCallHelpers::TrustedImm32(sizeof(void*) - static_cast<int64_t>(stackSize)), CCallHelpers::framePointerRegister, CCallHelpers::stackPointerRegister); > > > > Why the "-" here? > I think what I'm more curious about is why are we not subtracting > the entire stack size, but subtracting stack size minus 8 bytes What LLVM calls the stack size is the amount of stack space that the function will use. This does include saving of the frame pointer, which is saved above the actual frame. You just made me realize that this is a bug on ARM64 however, since we save the link register in the actual frame, in addition to the frame pointer. This should have crashed almost everything, I wonder why we don't see it happening...
Geoffrey Garen
Comment 7 2015-09-01 11:31:22 PDT
Comment on attachment 260335 [details] Patch r- because of the bug Saam and Basile noticed.
Basile Clement
Comment 8 2015-09-01 14:41:00 PDT
Created attachment 260384 [details] Patch
Basile Clement
Comment 9 2015-09-02 16:25:34 PDT
Created attachment 260452 [details] Patch
Basile Clement
Comment 10 2015-09-02 16:26:09 PDT
Comment on attachment 260452 [details] Patch This is the wrong patch.
Basile Clement
Comment 11 2015-09-02 16:27:27 PDT
Comment on attachment 260452 [details] Patch Actually, I don't know how to read, and this is the right patch.
Michael Saboff
Comment 12 2015-09-02 17:48:15 PDT
Comment on attachment 260452 [details] Patch r=me
Basile Clement
Comment 13 2015-09-03 17:25:24 PDT
Committed r189325: <http://trac.webkit.org/changeset/189325>
Note You need to log in before you can comment on or make changes to this bug.