[wasm-split] Share active table with caller modules

[aheejin]

The way we currently share the active table with secondary modules

binaryen/src/ir/module-splitting.cpp

Lines 1122 to 1156 in 2c2509b

	for (auto& [secondaryPtr, replacedElems] : moduleToReplacedElems) {
	Module& secondary = *secondaryPtr;
	// Import and export the active table if necessary. Unless we use an
	// existing table as an active table (e.g. because reference-types is
	// disabled) and that table was already being used by an existing indirect
	// call, shareImportableItems wasn't able to mark it as used in secondaries,
	// so we should export and import the active table here.
	auto secondaryTable =
	secondary.getTableOrNull(tableManager.activeTable->name);
	if (secondaryTable) {
	// In case it's already in the secondary module, sync the initial/max
	secondaryTable->initial = tableManager.activeTable->initial;
	secondaryTable->max = tableManager.activeTable->max;
	} else {
	secondaryTable =
	ModuleUtils::copyTable(tableManager.activeTable, secondary);
	makeImportExport(*tableManager.activeTable,
	*secondaryTable,
	"table",
	ExternalKind::Table);
	}
	if (tableManager.activeBase.global) {
	// Import and export the active table's base global if necessary. Unless
	// the base global was already being used elsewhere in secondaries,
	// shareImportableItems wasn't able to mark it as used in secondaries, so
	// we should export and import it here.
	auto* primaryGlobal = primary.getGlobal(tableManager.activeBase.global);
	auto* secondaryGlobal =
	secondary.getGlobalOrNull(tableManager.activeBase.global);
	if (!secondaryGlobal) {
	secondaryGlobal = ModuleUtils::copyGlobal(primaryGlobal, secondary);
	}
	makeImportExport(
	*primaryGlobal, *secondaryGlobal, "global", ExternalKind::Global);

has missed an important case in multi-split. This is one of the unexpected consequences of #8688.

Here the secondary module in this loop is a secondary module whose functions have been put in the active table, mostly by getTrampoline function. When we have a call from $A to $B and $B is split, getTrampoline puts $B on the active table, which will be converted to a placeholder in setupTablePatcing. So the code above share the active table (and its base global, if exists) with $B's module.

The problem is we didn't share the active global with $A's module. In a two-way split this is not a problem because $A is in the primary module. But in multi-split, $A's module itself is another secondary module, and this module needs to access the active table because it should call the placeholder for $B. This does it in indirectCallsToSecondaryFunctions.

---

I wish we can do the active table sharing for callee modules in indirectCallsToSecondaryFunctions too and get done with it, but we still need to share it in shareImportableItems for secondary modules that have functions to be replaced in the active table, because there is a case of existing secondary function names in the active table. See KEEP-NONE RUN line in the test below:
https://github.com/WebAssembly/binaryen/blob/main/test/lit/wasm-split/basic.wast

In this test, we reuse the existing table as the active table, which already has $foo in its element section. And that foo will be split. Because both bar and foo are split there is no cross-module call between them to process in indirectCallsToSecondaryFunctions. This won't be converted to trampolines because

binaryen/src/ir/module-splitting.cpp

Lines 947 to 958 in 5fcc1af

	// Ignore references to secondary functions that occur in the active segment
	// that will contain the imported placeholders. Indirect calls to table slots
	// initialized by that segment will already go to the right place once the
	// secondary module has been loaded and the table has been patched.
	std::unordered_set<RefFunc*> ignore;
	if (tableManager.activeSegment) {
	for (auto* expr : tableManager.activeSegment->data) {
	if (auto* ref = expr->dynCast<RefFunc>()) {
	ignore.insert(ref);
	}
	}
	}

and because there is no trampoline, this doesn't get to have a call instruction from primary->secondary, but we still need to share the active table with the secondary module, and we can't handle it in indirectCallsToSecondaryFunctions.

---

This fixes the error discussed in #8711 (comment).

[aheejin]

This was just extracted from the existing code in setupTablePatching.

[tlively]

Can we use an unordered set here?

[aheejin]

Hmm, yeah I think we can. I used std::set thinking we had to iterate on it below and the order mattered, but because we are iterating on modules and adding one export per module, the order won't really matter. Done.

[tlively]

It is slightly less code if CallIndirector owns activeTableUsingSecondaries. The downside is that have to access it with callIndirector.activeTableUsingSecondaries below. Historically I've more commonly used the pattern you have here though, so either way is fine with me.

[aheejin]

Done