Bug 172188

We just read random memory and call it, potentially. Fun stuff

<rdar://problem/32231828>

For example: ``` import Builder from '../Builder.js' import * as assert from '../assert.js' { function makeInstance() { const tableDescription = {initial: 1, element: "anyfunc"}; const builder = new Builder() .Type() .Func([], "void") .End() .Import() .Table("imp", "table", tableDescription) .End() .Function().End() .Export() .Function("foo") .End() .Code() .Function("foo", {params:["i32"], ret:"void"}) .GetLocal(0) // parameter to call .GetLocal(0) // call index .CallIndirect(0, 0) // calling function of type [] => 'void' .Return() .End() .End(); const bin = builder.WebAssembly().get(); const module = new WebAssembly.Module(bin); const table = new WebAssembly.Table(tableDescription); return {instance: new WebAssembly.Instance(module, {imp: {table}}), table}; } function makeInstance2() { const tableDescription = {initial: 1, element: "anyfunc"}; const builder = new Builder() .Type() .End() .Import() .Function("imp", "f", {params:[], ret:"void"}) .End() .Function().End() .Export() .Function("foo") .End() .Code() .Function("foo", {params: [], ret: "void" }) .Call(0) .Return() .End() .End(); const bin = builder.WebAssembly().get(); const module = new WebAssembly.Module(bin); return new WebAssembly.Instance(module, {imp: {f: function() { print("Inside f"); }}}); } const {instance: i1, table: t1} = makeInstance(); const foo = i1.exports.foo; const i2 = makeInstance2(); t1.set(0, i2.exports.foo); foo(0); } ```

Created attachment 310308 [details] WIP I'm thinking of moving to a thunk approach, so we're not stuck with this branchiness. Just putting this here since I think it works.

The thunk approach could be like this: A table has three states: - NoInstance: not yet tied to an instance, so we hold off on making any thunks, since it's not yet callable from wasm - SingleInstance: Now, all functions in the table only need thunks to context switch if they're from a different instance - MultipleInstances: now all functions need context switching thunks regardless, since we don't know what instance is calling us. We could have the thunk branch to see if it's actually performing a context switch.

(In reply to Saam Barati from comment #5) > The thunk approach could be like this: > > A table has three states: > - NoInstance: not yet tied to an instance, so we hold off on making any > thunks, since it's not yet callable from wasm > - SingleInstance: Now, all functions in the table only need thunks to > context switch if they're from a different instance > - MultipleInstances: now all functions need context switching thunks > regardless, since we don't know what instance is calling us. We could have > the thunk branch to see if it's actually performing a context switch. Ima save thunk land for: https://bugs.webkit.org/show_bug.cgi?id=172197

Created attachment 310330 [details] WIP Might be done.

Created attachment 310332 [details] patch

Created attachment 310336 [details] patch added file to cmakelists.

Created attachment 310340 [details] patch

Comment on attachment 310340 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310340&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1155 > + BasicBlock* continuation = m_proc.addBlock(); > + BasicBlock* doContextSwitch = m_proc.addBlock(); These should probably be switched for better block ordering.

Comment on attachment 310340 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310340&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1170 > + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) { I think as a follow-up to this patch I'd rather have this be outlined, and shared between all modules. Kinda like what we do for direct wasm->wasm calls, but cleaned up. I think the code duplication and compile cost aren't worth it, because I don't think inlining exposes any perf win here. Maybe add a FIXME?

Comment on attachment 310340 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310340&action=review >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1170 >> + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) { > > I think as a follow-up to this patch I'd rather have this be outlined, and shared between all modules. Kinda like what we do for direct wasm->wasm calls, but cleaned up. I think the code duplication and compile cost aren't worth it, because I don't think inlining exposes any perf win here. > > Maybe add a FIXME? I think long term we just want to do this: https://bugs.webkit.org/show_bug.cgi?id=172197

Comment on attachment 310340 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310340&action=review r=me. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1155 >> + BasicBlock* doContextSwitch = m_proc.addBlock(); > > These should probably be switched for better block ordering. I don't think this matters. Doesn't B3 pick the ordering it wants later anyway? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1164 > + PatchpointValue* patchpoint = doContextSwitch->appendNew<PatchpointValue>(m_proc, B3::Void, origin()); I think you need to say this writes pinned.

landed in: https://trac.webkit.org/changeset/217017/webkit