Bug 172316 – We overwrite the callee save space on the stack when throwing stack overflow from wasm

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryan Haddad]

** The following JSC stress test failures have been introduced:
	wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison
	wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-tls-context

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/1494

[Ryan Haddad]

Created attachment 310553 [details]
Crash log

[Saam Barati]

I’ll look into this tomorrow.

[Saam Barati]

Created attachment 310746 [details]
patch

[Saam Barati]

Created attachment 310753 [details]
patch

[Mark Lam]

Comment on attachment 310753 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:23
> +          move fp, sp
> +          sub maxNumCalleeSaveSpace, sp

I think you want to use a ternary form of sub, or use a temp register.  You don't want to pop sp all the way to fp and then reserving back some space.  This is because a signal handler may fire at that moment between the 2 instructions, and overwrite anything below the sp.  Instead, you should ensure that the sp never drops that low.

[Saam Barati]

Sounds good.

[Saam Barati]

Created attachment 310782 [details]
patch

[Mark Lam]

Comment on attachment 310782 [details]
patch

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

r=me with ChangeLog fixed.

> Source/JavaScriptCore/ChangeLog:22
> +          add maxNumCalleeSaveSpace, fp, sp

I think you mean -maxNumCalleeSaveSpace here.

> Source/JavaScriptCore/wasm/WasmThunks.cpp:98
> +    jit.addPtr(CCallHelpers::TrustedImm32(-stackSpace), GPRInfo::callFrameRegister, MacroAssembler::stackPointerRegister);

I was worried that x86_64 will reduce this to a pair of binary ops (move fp, sp; add -X, sp), but I see that it really does perform a ternary operation using an lea instruction instead.  So, all is well.

[Saam Barati]

Created attachment 310817 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 310817 [details]
patch for landing

Clearing flags on attachment: 310817

Committed r217198: <http://trac.webkit.org/changeset/217198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.