Bug 148659

Summary: JavaScript functions should restore the stack pointer after a call
Product: WebKit Reporter: Basile Clement <basile_clement>
Component: JavaScriptCoreAssignee: Basile Clement <basile_clement>
Status: RESOLVED FIXED    
Severity: Normal CC: msaboff, saam
Priority: P2    
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+

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>