jsc-tailcall: JavaScript functions should restore the stack pointer after a call

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Basile Clement]

The LLint and baseline JIT are already doing this, we statically know the stack size in the DFG, and we can get it from the LLVM stackmap in FTL code - so this should be an easy change.

Once https://bugs.webkit.org/show_bug.cgi?id=146845 is taken care of as well, this will allow us to:

 1) Get rid of the arity fixup return thunk (since the caller now knows where the stack pointer should be as an offset of its base pointer)

 2) Actually implement tail calls since they can shrink or extend the stack in unpredictable ways.

[Basile Clement]

Created attachment 256709 [details]
Patch using StackMap size

This uses the stackmap size from LLVM to restore the stack pointer to where it should be in the patchpoint. I am interested in hearing opinions on the following two points:

 - We may want to adapt the default patchpoint size to accommodate the additional lea opcode.

 - LLVM provides a stackmap/stackrestore intrinsic that we should be able to use. I am unsure what guarantees we would have with this; in particular, I fear LLVM spilling values onto the stack before the call but after saving the stack pointer, performing the call, then trying to restore those values using the stack pointer as it is supposed to be callee-save. I did not see this happen even when testing that approach with high register pressure (LLVM ends up using bp-based offsets for the spilling), but again, I am unsure how much we can rely on this. I believe messing up with the stack pointer in the patchpoint is better because it makes it completely transparent to LLVM.

[Basile Clement]

(In reply to comment #1)
> Created attachment 256709 [details]
> Patch using StackMap size
> 
> This uses the stackmap size from LLVM to restore the stack pointer to where
> it should be in the patchpoint. I am interested in hearing opinions on the
> following two points:
> 
>  - We may want to adapt the default patchpoint size to accommodate the
> additional lea opcode.
> 
>  - LLVM provides a stackmap/stackrestore intrinsic that we should be able to
> use. I am unsure what guarantees we would have with this; in particular, I
> fear LLVM spilling values onto the stack before the call but after saving
> the stack pointer, performing the call, then trying to restore those values
> using the stack pointer as it is supposed to be callee-save. I did not see
> this happen even when testing that approach with high register pressure
> (LLVM ends up using bp-based offsets for the spilling), but again, I am
> unsure how much we can rely on this. I believe messing up with the stack
> pointer in the patchpoint is better because it makes it completely
> transparent to LLVM.

After talking offline with fpizlo, the stackmap size approach looks like the best one.

[Michael Saboff]

Comment on attachment 256709 [details]
Patch using StackMap size

View in context: https://bugs.webkit.org/attachment.cgi?id=256709&action=review

r=me with suggested name change to a function.

> Source/JavaScriptCore/ftl/FTLJSCall.cpp:54
> +void JSCall::emit(CCallHelpers& jit, unsigned stackSize)

Instead of calling this "emit", how about "emitRestoreStackPointer"?

[Basile Clement]

(In reply to comment #3)
> Comment on attachment 256709 [details]
> Patch using StackMap size
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256709&action=review
> 
> r=me with suggested name change to a function.
> 
> > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54
> > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize)
> 
> Instead of calling this "emit", how about "emitRestoreStackPointer"?

FTR, after discussion offline, we'll keep "emit" since this is doing more than just restoring the stack pointer.

[Basile Clement]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 256709 [details]
> > Patch using StackMap size
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=256709&action=review
> > 
> > r=me with suggested name change to a function.
> > 
> > > Source/JavaScriptCore/ftl/FTLJSCall.cpp:54
> > > +void JSCall::emit(CCallHelpers& jit, unsigned stackSize)
> > 
> > Instead of calling this "emit", how about "emitRestoreStackPointer"?
> 
> FTR, after discussion offline, we'll keep "emit" since this is doing more
> than just restoring the stack pointer.

Committed r186815 <http://trac.webkit.org/changeset/186815>.