wasm-split fuzzing and reusing an existing Table

[kripken]

I seem to recall we discussed using a new Table in wasm-split, when reference types is set? Atm it looks like we reuse the table if one exists:

https://github.com/WebAssembly/binaryen/blob/28e849b91fab4fb697c27206b7d6f2c090e3519c/src/ir/module-splitting.cpp#L186-L191

The fuzzer errored on this, with exports like these:

 (func $0
 )
 (func $1
  (table.set $0
   (i32.const 1)
   (ref.null nofunc)
  )
  (unreachable)
 )
 (func $2
 )

After splitting these three out, we end up with call_indirects in all three in the primary module. The secondary module's elem writes the proper function pointers, but when we call $1 we trample some of that data, leading to the third export trapping. That is, it is unsafe to use the table for normal stuff and also wasm-split stuff, without the two being aware of each other.

cc @tlively @aheejin

[tlively]

This logic is responsible for skipping the reuse of an existing table and will cause a new table to be created on demand:

https://github.com/WebAssembly/binaryen/blob/main/src/ir/module-splitting.cpp#L175-L178

The fuzzer should make sure that condition is always satisfied to ensure it always gets a fresh table.

[kripken]

@tlively Ah, I see 😄 Looks like that checks for an emscripten-style table import, and if it exists, reuses the table. And we do have that in our test suite... so we would need to skip those files, but (1) it is several files, and (2) they are emscripten testcases, which we may add more of, so this seems like a long-term issue.

Why do we need that special logic? Is there a problem with emscripten using a second table?

[tlively]

@dschuff added the special casing of emscripten tables in https://github.com/WebAssembly/binaryen/pull/7050. It sounds like Emscripten's loading code couldn't handle the second table at the time. @aheejin, is this something we could fix on the Emscripten side so we can remove the special case here?

[aheejin]

I'm not familiar with why that was done that way wrt dynamic loading. @dschuff Can you point to more context?

[dschuff]

When loading a split module, the loader code around here patches the indirect function table, overwriting the pointers to the proxy with the real functions. When wasm-split creates a second table for this purpose the loader code just needs to figure out which table to patch.