Bug 148659

Summary:

JavaScript functions should restore the stack pointer after a call

Product:

WebKit

Reporter:

Basile Clement <basile_clement>

Component:

JavaScriptCore

Assignee:

Basile Clement <basile_clement>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

148661

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	msaboff: review+

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	Details Formatted Diff Diff
Patch (132.61 KB, patch) 2015-08-31 17:05 PDT, Basile Clement	no flags	Details Formatted Diff Diff
Patch (7.61 KB, patch) 2015-08-31 17:05 PDT, Basile Clement	no flags	Details Formatted Diff Diff
Patch (7.87 KB, patch) 2015-09-01 14:41 PDT, Basile Clement	no flags	Details Formatted Diff Diff
Patch (8.99 KB, patch) 2015-09-02 16:25 PDT, Basile Clement	msaboff: review+	Details Formatted Diff Diff
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.