[JSC] Store Wasm::Instance* in |codeBlock| slot by Constellation · Pull Request #8815 · WebKit/WebKit

Constellation · 2023-01-19T04:32:43Z

`c9c2f34`

[JSC] Store Wasm::Instance* in |codeBlock| slot
https://bugs.webkit.org/show_bug.cgi?id=250822
rdar://104410328

Reviewed by Mark Lam.

This patch fixes wasm calling convention problems. The new one becomes much simpler and robust (and fixing existing bugs).

1. We remove vm.wasmContext.instance field. Previously, it was randomly configured and a lot code is missing the update of this thing (for example, wasm context switching
   inside wasm -> wasm call missed the update). We should not have this kind of "global" variable and instead we should query to CallFrame about Wasm::Instance* of the current CallFrame*.
2. Each wasm function stores Wasm::Instance* to the CallFrame. We use |codeBlock| slot for that purpose. Since it is next to |callee| slot, we can use storePairPtr in ARM64, no code size increase.
   Plus, because we are already writing |callee| slot, it does not add new performance problem. This change makes CallFrame::lexicalGlobalObjectFromWasmCallee super simple since we can just get
   this slot from CallFrame instead of getting it from vm.wasmContext.instance. And it also makes Interpreter::unwind much simpler since we no longer need to book-keep this variable in unwinding case.
   And this also contributes to code size reduction in Wasm IC since we no longer need to have store and load code for vm.wasmContext.instance.
3. We use |codeBlock| slot for Wasm::Instance*. However, Wasm LLInt already used it for Wasm::Callee* (wasm's codeblock). This patch allocates 2 internal slots for Wasm::LLInt code and use
   this slot for Wasm::Callee* instead. This slot is "WasmCodeBlock".
4. We revise the use of |this| slot for wasm functions. Previously, it was randomly used. But we use this slot to anchor JSWebAssemblyInstance from conservative GC root.
   While we are keeping Wasm::Instance* in the stack, it is not GC-managed cell. To keep Wasm functions alive while running, we need to anchor JSWebAssemblyInstance* from conservative GC root.
   We use this slot in three cases. (1) Calling wasm function from JS world / C++ world so that we need to keep wasm function alive. (2) We wasm-tail-call to a new function. Since tail-call can wipe the previous
   frame, if it is the entrance frame created by (1), we miss the anchor. Conservatively, we always store this cell in tail-call case. (3) And we do this when calling wasm function indirectly.
   This could switch to a new wasm instance, so we should keep a new instance anchored from the stack.

This change wipes a lot of weird things in wasm and makes calling convention much simpler.

* Source/JavaScriptCore/interpreter/Interpreter.cpp:
(JSC::UnwindFunctor::operator() const):
* Source/JavaScriptCore/interpreter/ProtoCallFrame.h:
* Source/JavaScriptCore/llint/WebAssembly.asm:
* Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h:
(JSC::Wasm::ExpressionType>::AirIRGeneratorBase):
* Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::insertConstants):
* Source/JavaScriptCore/wasm/WasmCallingConvention.h:
* Source/JavaScriptCore/wasm/WasmIRGeneratorHelpers.h:
(JSC::Wasm::emitCatchPrologueShared):
* Source/JavaScriptCore/wasm/js/JSToWasm.cpp:
(JSC::Wasm::createJSToWasmWrapper):
* Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/259139@main

e93a5c3

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	❌ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

webkit-early-warning-system · 2023-01-19T04:34:33Z

EWS run on previous version of this PR (hash b51a8b2)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

Constellation · 2023-01-19T04:41:26Z

Ah, will do a bit more drastic change.

webkit-early-warning-system · 2023-01-19T07:35:30Z

EWS run on previous version of this PR (hash e43938c)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-01-19T07:36:36Z

EWS run on previous version of this PR (hash ad91996)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	❌ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	✅ 🧪 mac-wk1	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-19T19:20:34Z

EWS run on previous version of this PR (hash 1fec380)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-19T20:51:28Z

EWS run on previous version of this PR (hash 3184e4b)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-19T22:00:23Z

EWS run on previous version of this PR (hash c2820e4)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-20T00:11:10Z

EWS run on previous version of this PR (hash b0ecd3f)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	⏳ 🧪 mac-wk1	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	⏳ 🛠 watch	⏳ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-01-20T00:14:32Z

EWS run on previous version of this PR (hash 53df791)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	⏳ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-01-20T00:47:28Z

EWS run on previous version of this PR (hash 75abe4a)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

MenloDorian · 2023-01-20T02:47:23Z

Need some clarification on what you mean in parts of the commit msg.

We remove vm.wasmContext.instance field. It is randomly configured and a lot code is missing the update of this thing (for example, wasm context switching
inside wasm -> wasm call missed the update). We should not have this kind of "global" variable and instead we should query to CallFrame about Wasm::Instance* of the current CallFrame*.

/It is randomly configured/Previously, it is randomly configured/

We use |codeBlock| slot for Wasm::Instance*. However, Wasm LLInt already used it for Wasm::Callee* (wasm's codeblock). This patch allocates 2 internal slots for Wasm::LLInt code and use

this slot for Wasm::Callee* instead. This slot is "WasmCodeBlock".

2 internal slots in what? I'm not sure I understand what is claimed here: it sounds like we'll consistently use |codeBlock| for the Wasm::Instance*. And we're now going to store the Wasm::Callee* where? Can you clarify?

We revise the use of |this| slot for wasm functions. Previously, it was randomly used. But we use this slot to anchor JSWebAssemblyInstance from conservative GC root.
While we are keeping Wasm::Instance* in the stack, it is not GC-managed cell. To keep Wasm functions alive while running, we need to anchor JSWebAssemblyInstance* from conservative GC root.
We use this slot in three cases.

Regarding these 3 cases, are you saying that this is how we previously do it? Or are you saying that this is the new way of doing it? Can you make that clear?

(1) Calling wasm function from JS world / C++ world so that we need to keep it alive.

What is the "it" that we need to keep alive? Do you mean the JSWebAssemblyInstance*, and that the new code will keep it alive here by stashing it in |this|?

(2) We wasm-tail-call to a new function. Since tail-call can wipe the previous
frame, if it is the entrance frame created by (1), we miss the anchor. Conservatively, we always store this cell in tail-call case.

Do you mean we'll move the JSWebAssemblyInstance* from the caller's |this| to the tail call callee's |this|? Or do you mean something else?

(3) And we do this when calling wasm function indirectly.
This could switch to a new wasm instance, so we should keep a new instance anchored from the stack.

Here, do you mean that when A calls B, A needs to stash B's JSWebAssemblyInstance* in the |this| slot for B's frame?

Jumping into the code now ...

Constellation · 2023-01-20T03:59:02Z

Need some clarification on what you mean in parts of the commit msg.

We remove vm.wasmContext.instance field. It is randomly configured and a lot code is missing the update of this thing (for example, wasm context switching
inside wasm -> wasm call missed the update). We should not have this kind of "global" variable and instead we should query to CallFrame about Wasm::Instance* of the current CallFrame*.

/It is randomly configured/Previously, it is randomly configured/

We use |codeBlock| slot for Wasm::Instance*. However, Wasm LLInt already used it for Wasm::Callee* (wasm's codeblock). This patch allocates 2 internal slots for Wasm::LLInt code and use

this slot for Wasm::Callee* instead. This slot is "WasmCodeBlock".

2 internal slots in what? I'm not sure I understand what is claimed here: it sounds like we'll consistently use |codeBlock| for the Wasm::Instance*.

LLInt's slots. See WebAssembly.asm's code doing that. As the same to LLInt's callee-save register area, we allocate 2 slots, and use it for WasmCodeBlock purpose.

And we're now going to store the Wasm::Callee* where? Can you clarify?

WasmCodeBlock slot. "this slot for Wasm::Callee* instead. This slot is "WasmCodeBlock""

We revise the use of |this| slot for wasm functions. Previously, it was randomly used. But we use this slot to anchor JSWebAssemblyInstance from conservative GC root.
While we are keeping Wasm::Instance* in the stack, it is not GC-managed cell. To keep Wasm functions alive while running, we need to anchor JSWebAssemblyInstance* from conservative GC root.
We use this slot in three cases.

Regarding these 3 cases, are you saying that this is how we previously do it? Or are you saying that this is the new way of doing it? Can you make that clear?

No, they are the new way in this patch.

(1) Calling wasm function from JS world / C++ world so that we need to keep it alive.

What is the "it" that we need to keep alive? Do you mean the JSWebAssemblyInstance*, and that the new code will keep it alive here by stashing it in |this|?

"wasm function"

(2) We wasm-tail-call to a new function. Since tail-call can wipe the previous
frame, if it is the entrance frame created by (1), we miss the anchor. Conservatively, we always store this cell in tail-call case.

Do you mean we'll move the JSWebAssemblyInstance* from the caller's |this| to the tail call callee's |this|? Or do you mean something else?

Not move. Store JSWebAssemblyInstance* of the target function to |this| of the callFrame.

(3) And we do this when calling wasm function indirectly.
This could switch to a new wasm instance, so we should keep a new instance anchored from the stack.

Here, do you mean that when A calls B, A needs to stash B's JSWebAssemblyInstance* in the |this| slot for B's frame?

No. Just storing B's JSWebAssemblyInstance* into B's |this| slot (since |this| is an argument).
"We use this slot in three cases.". And in the other cases, we are not storing this value to |this| slot since it is not necessary (e.g. If A and B are the same instance, then A is already anchoring it, so B does not need to do it again).

Jumping into the code now ...

MenloDorian

r=me

Source/JavaScriptCore/interpreter/CallFrame.cpp

Source/JavaScriptCore/interpreter/Interpreter.cpp

Source/JavaScriptCore/interpreter/ProtoCallFrame.h

Source/JavaScriptCore/llint/WebAssembly.asm

Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h

Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp

Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp

Constellation

Thanks!

Source/JavaScriptCore/interpreter/CallFrame.cpp

Source/JavaScriptCore/interpreter/Interpreter.cpp

Source/JavaScriptCore/interpreter/ProtoCallFrame.h

Source/JavaScriptCore/llint/WebAssembly.asm

Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp

Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h

Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp

Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h

webkit-early-warning-system · 2023-01-20T06:32:28Z

EWS run on current version of this PR (hash b7d4d04)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
🧪 style	🛠 ios	🛠 mac	🛠 wpe	⏳ 🛠 🧪 win
	🛠 ios-sim	🛠 mac-AS-debug	🛠 gtk	⏳ 🛠 wincairo
🧪 webkitperl	🧪 ios-wk2	🧪 api-mac	🧪 gtk-wk2
	🧪 api-ios	🧪 mac-wk1	🧪 api-gtk
🛠 🧪 jsc	🛠 tv	🧪 mac-wk2	🛠 jsc-armv7
🛠 🧪 jsc-arm64	🛠 tv-sim	🧪 mac-AS-debug-wk2	🧪 jsc-armv7-tests
	🛠 watch	🧪 mac-wk2-stress	🛠 jsc-mips
	🛠 watch-sim		🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-20T06:32:30Z

EWS run on previous version of this PR (hash b7d4d04)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
✅ 🛠 🧪 jsc	✅ 🛠 tv	⏳ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	~~🧪 jsc-armv7-tests~~
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-01-20T06:50:45Z

EWS run on previous version of this PR (hash 014af4f)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	❌ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	❌ 🛠 watch	✅ 🧪 mac-wk2-stress	❌ 🛠 jsc-mips
	✅ 🛠 watch-sim		❌ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-01-20T16:05:16Z

EWS run on current version of this PR (hash e93a5c3)

Details

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	❌ 🧪 gtk-wk2
	~~🧪 api-ios~~	~~🧪 mac-wk1~~	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

kmiller68 · 2023-01-20T16:28:01Z

Source/JavaScriptCore/interpreter/CallFrameInlines.h

I almost think we should have a second enum mirroring CallFrameSlot that has all the wasm slots. But we can do that another day. Alternatively, we could rename this to CallFrameSlot::jsCodeBlockOrWasmInstance but I think having two enums is clearer.

Source/JavaScriptCore/llint/WebAssembly.asm

kmiller68 · 2023-01-20T16:36:52Z

Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h

Ditto, on the comment here.

kmiller68 · 2023-01-20T16:37:38Z

Source/JavaScriptCore/wasm/WasmIRGeneratorHelpers.h

Ditto on comment here.

kmiller68 · 2023-01-20T16:37:52Z

Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp

Ditto on comment.

kmiller68 · 2023-01-20T16:38:18Z

Source/JavaScriptCore/wasm/js/JSToWasm.cpp

Ditto on comment.

kmiller68 · 2023-01-20T16:39:03Z

Source/JavaScriptCore/wasm/js/JSToWasm.cpp

Ditto on comment.

https://bugs.webkit.org/show_bug.cgi?id=250822 rdar://104410328 Reviewed by Mark Lam. This patch fixes wasm calling convention problems. The new one becomes much simpler and robust (and fixing existing bugs). 1. We remove vm.wasmContext.instance field. Previously, it was randomly configured and a lot code is missing the update of this thing (for example, wasm context switching inside wasm -> wasm call missed the update). We should not have this kind of "global" variable and instead we should query to CallFrame about Wasm::Instance* of the current CallFrame*. 2. Each wasm function stores Wasm::Instance* to the CallFrame. We use |codeBlock| slot for that purpose. Since it is next to |callee| slot, we can use storePairPtr in ARM64, no code size increase. Plus, because we are already writing |callee| slot, it does not add new performance problem. This change makes CallFrame::lexicalGlobalObjectFromWasmCallee super simple since we can just get this slot from CallFrame instead of getting it from vm.wasmContext.instance. And it also makes Interpreter::unwind much simpler since we no longer need to book-keep this variable in unwinding case. And this also contributes to code size reduction in Wasm IC since we no longer need to have store and load code for vm.wasmContext.instance. 3. We use |codeBlock| slot for Wasm::Instance*. However, Wasm LLInt already used it for Wasm::Callee* (wasm's codeblock). This patch allocates 2 internal slots for Wasm::LLInt code and use this slot for Wasm::Callee* instead. This slot is "WasmCodeBlock". 4. We revise the use of |this| slot for wasm functions. Previously, it was randomly used. But we use this slot to anchor JSWebAssemblyInstance from conservative GC root. While we are keeping Wasm::Instance* in the stack, it is not GC-managed cell. To keep Wasm functions alive while running, we need to anchor JSWebAssemblyInstance* from conservative GC root. We use this slot in three cases. (1) Calling wasm function from JS world / C++ world so that we need to keep wasm function alive. (2) We wasm-tail-call to a new function. Since tail-call can wipe the previous frame, if it is the entrance frame created by (1), we miss the anchor. Conservatively, we always store this cell in tail-call case. (3) And we do this when calling wasm function indirectly. This could switch to a new wasm instance, so we should keep a new instance anchored from the stack. This change wipes a lot of weird things in wasm and makes calling convention much simpler. * Source/JavaScriptCore/interpreter/Interpreter.cpp: (JSC::UnwindFunctor::operator() const): * Source/JavaScriptCore/interpreter/ProtoCallFrame.h: * Source/JavaScriptCore/llint/WebAssembly.asm: * Source/JavaScriptCore/wasm/WasmAirIRGeneratorBase.h: (JSC::Wasm::ExpressionType>::AirIRGeneratorBase): * Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp: (JSC::Wasm::B3IRGenerator::insertConstants): * Source/JavaScriptCore/wasm/WasmCallingConvention.h: * Source/JavaScriptCore/wasm/WasmIRGeneratorHelpers.h: (JSC::Wasm::emitCatchPrologueShared): * Source/JavaScriptCore/wasm/js/JSToWasm.cpp: (JSC::Wasm::createJSToWasmWrapper): * Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): Canonical link: https://commits.webkit.org/259139@main

webkit-commit-queue · 2023-01-20T16:42:05Z

Committed 259139@main (c9c2f34): https://commits.webkit.org/259139@main

Reviewed commits have been landed. Closing PR #8815 and removing active labels.

Constellation requested a review from a team as a code owner January 19, 2023 04:32

Constellation self-assigned this Jan 19, 2023

Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Jan 19, 2023

Constellation marked this pull request as draft January 19, 2023 07:33

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from b51a8b2 to e43938c Compare January 19, 2023 07:35

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from e43938c to ad91996 Compare January 19, 2023 07:36

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 19, 2023

Constellation removed the merging-blocked Applied to prevent a change from being merged label Jan 19, 2023

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from ad91996 to 1fec380 Compare January 19, 2023 19:18

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from 1fec380 to 3184e4b Compare January 19, 2023 20:45

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from 3184e4b to c2820e4 Compare January 19, 2023 22:00

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from c2820e4 to b0ecd3f Compare January 20, 2023 00:06

Constellation marked this pull request as ready for review January 20, 2023 00:06

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from b0ecd3f to 53df791 Compare January 20, 2023 00:14

Constellation changed the title [JSC] Store Wasm::Instance* in |this| slot instead of JSWebAssemblyInstance* [JSC] Store Wasm::Instance* in |codeBlock| slot Jan 20, 2023

Constellation mentioned this pull request Jan 20, 2023

[JSC] Fast JS-to-Wasm call from FTL #8607

Merged

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from 53df791 to 75abe4a Compare January 20, 2023 00:46

MenloDorian approved these changes Jan 20, 2023

View reviewed changes

Constellation commented Jan 20, 2023

View reviewed changes

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from 75abe4a to b7d4d04 Compare January 20, 2023 06:31

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from b7d4d04 to 014af4f Compare January 20, 2023 06:45

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 20, 2023

Constellation removed the merging-blocked Applied to prevent a change from being merged label Jan 20, 2023

Constellation force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from 014af4f to e93a5c3 Compare January 20, 2023 16:03

Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 20, 2023

kmiller68 reviewed Jan 20, 2023

View reviewed changes

webkit-early-warning-system force-pushed the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch from e93a5c3 to c9c2f34 Compare January 20, 2023 16:41

webkit-early-warning-system merged commit c9c2f34 into WebKit:main Jan 20, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 20, 2023

Constellation deleted the eng/JSC-Store-WasmInstance-in-this-slot-instead-of-JSWebAssemblyInstance branch January 20, 2023 16:45

Conversation

Constellation commented Jan 19, 2023 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

c9c2f34

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Constellation commented Jan 19, 2023

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MenloDorian commented Jan 20, 2023

Uh oh!

Constellation commented Jan 20, 2023

Uh oh!

MenloDorian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Constellation left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmiller68 Jan 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Constellation commented Jan 19, 2023 •

edited by webkit-early-warning-system

Loading

`c9c2f34`

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 19, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading

webkit-early-warning-system commented Jan 20, 2023 •

edited

Loading