[wasm-split] Fix table naming conflicts#8708
Conversation
After WebAssembly#8688, we split module elements, including tables, earlier than `indirectCallsToSecondaryFunctions`, which calls `getSlot`, which calls `makeTable`. But when making a table, we only try to get a valid name within the primary module: https://github.com/WebAssembly/binaryen/blob/2f1f55aef6d9adfa6fdc2c25e46d202232dbf6e2/src/ir/module-splitting.cpp#L235-L238 If an existing table's name was `0` and it was moved to a secondary module in `shareImportable` already, this will happily create an active table with the name `0` again. And in `setupTablePatching`, because the secondary module already has `0`, the active table will not be exported / imported there: https://github.com/WebAssembly/binaryen/blob/2f1f55aef6d9adfa6fdc2c25e46d202232dbf6e2/src/ir/module-splitting.cpp#L1103-L1117 But this existing table is NOT the active table, and this table's type may not even be `funcref`. This fixes `makeTable` so that it makes a table name that does not collide with any table names not only in the primary module but all secondary modules.
| ;; PRIMARY: (module | ||
| ;; PRIMARY-NEXT: (type $0 (func)) | ||
| ;; PRIMARY-NEXT: (import "placeholder.deferred" "0" (func $placeholder_0)) | ||
| ;; PRIMARY-NEXT: (table $0 1 funcref) |
There was a problem hiding this comment.
Table names are not actually shown in the output because its hasExternalName is false. But without this patch this test crashes because it tries to reuse the existing externref table in the secondary module as the active table.
There was a problem hiding this comment.
Are you saying that $0 is not the internal name of the table? Why are we emitting a name at all, then?
| for (auto& secondary : secondaries) { | ||
| if (secondary->getTableOrNull(test)) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
We don't want to iterate through all the secondaries multiple times in case it takes several iterations to find a good name. Let's iterate through them just once and create a set of all the table names we have to avoid.
There was a problem hiding this comment.
But we only create an active table once, no?
There was a problem hiding this comment.
Right. I mean that we should create this set just once at the beginning of makeTable. getValidName() can call the predicate an arbitrary number of times until it finds a valid name, so we want to avoid that being pathologically expensive.
| ;; PRIMARY: (module | ||
| ;; PRIMARY-NEXT: (type $0 (func)) | ||
| ;; PRIMARY-NEXT: (import "placeholder.deferred" "0" (func $placeholder_0)) | ||
| ;; PRIMARY-NEXT: (table $0 1 funcref) |
There was a problem hiding this comment.
It looks like the new table is still called $0?
There was a problem hiding this comment.
#8708 (comment)
We can set hasExternalName true depending on the debug information. Given that there will be only one active table, I'm not sure if it's worth it, but we can try, like even naming it "active_table" and make it shown in debug mode. (But this would be better as a follow-up, if we do it)
After #8688, we split module elements, including tables, earlier than
indirectCallsToSecondaryFunctions, which callsgetSlot, which callsmakeTable. But when making a table, we only try to get a valid name within the primary module:binaryen/src/ir/module-splitting.cpp
Lines 235 to 238 in 2f1f55a
If an existing table's name was
0and it was moved to a secondary module inshareImportablealready, this will happily create an active table with the name0again. And insetupTablePatching, because the secondary module already has0, the active table will not be exported / imported there:binaryen/src/ir/module-splitting.cpp
Lines 1103 to 1117 in 2f1f55a
But this existing table is NOT the active table, and this table's type may not even be
funcref.This fixes
makeTableso that it makes a table name that does not collide with any table names not only in the primary module but all secondary modules.This also disables
Splitfuzzer for now; I'm finding more bugs, so I'll reenable it after it is more stabilized.