Bug 148659 - JavaScript functions should restore the stack pointer after a call
Summary: JavaScript functions should restore the stack pointer after a call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Basile Clement
URL:
Keywords:
Depends on:
Blocks: 148661
  Show dependency tree / graph
 
Reported: 2015-08-31 17:02 PDT by Basile Clement
Modified: 2015-09-03 17:25 PDT (History)
2 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Basile Clement 2015-08-31 17:02:57 PDT 
...
Comment 1 Basile Clement 2015-08-31 17:04:41 PDT 
Created attachment 260333 [details]
Patch
Comment 2 Basile Clement 2015-08-31 17:05:19 PDT 
Created attachment 260334 [details]
Patch
Comment 3 Basile Clement 2015-08-31 17:05:48 PDT 
Created attachment 260335 [details]
Patch
Comment 4 Saam Barati 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?
Comment 5 Saam Barati 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
Comment 6 Basile Clement 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...
Comment 7 Geoffrey Garen 2015-09-01 11:31:22 PDT 
Comment on attachment 260335 [details]
Patch

r- because of the bug Saam and Basile noticed.
Comment 8 Basile Clement 2015-09-01 14:41:00 PDT 
Created attachment 260384 [details]
Patch
Comment 9 Basile Clement 2015-09-02 16:25:34 PDT 
Created attachment 260452 [details]
Patch
Comment 10 Basile Clement 2015-09-02 16:26:09 PDT 
Comment on attachment 260452 [details]
Patch

This is the wrong patch.
Comment 11 Basile Clement 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.
Comment 12 Michael Saboff 2015-09-02 17:48:15 PDT 
Comment on attachment 260452 [details]
Patch

r=me
Comment 13 Basile Clement 2015-09-03 17:25:24 PDT 
Committed r189325: <http://trac.webkit.org/changeset/189325>