Skip to content

[wasm-split] Fix table naming conflicts #8708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aheejin merged 8 commits into from
May 15, 2026
+72 −6
Merged

[wasm-split] Fix table naming conflicts #8708

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9abfe78
[wasm-split] Fix table naming conflicts
aheejin May 15, 2026
d38d9fa
Disable Split fuzzer
aheejin May 15, 2026
57d63fc
Make a set of secondary names first
aheejin May 15, 2026
24a4d91
Use -S and cat
aheejin May 15, 2026
8e621fc
Merge branch 'main' into wasm_split_fuzz_fix
aheejin May 15, 2026
cf68bab
Comment
aheejin May 15, 2026
2273159
Update src/ir/module-splitting.cpp
aheejin May 15, 2026
23a296a
Merge branch 'main' into wasm_split_fuzz_fix
aheejin May 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion scripts/fuzz_opt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2525,7 +2525,7 @@ def handle(self, wasm):
TrapsNeverHappen(),
CtorEval(),
Merge(),
Split(),
# Split(), # Will reenable after stabilized
RoundtripText(),
ClusterFuzz(),
Two(),
Expand Down
24 changes: 19 additions & 5 deletions src/ir/module-splitting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,15 @@ struct TableSlotManager {
Expression* makeExpr(Module& module);
};
Module& module;
const std::vector<std::unique_ptr<Module>>& secondaries;
Table* activeTable = nullptr;
ElementSegment* activeSegment = nullptr;
Slot activeBase;
std::map<Name, Slot> funcIndices;
std::vector<ElementSegment*> activeTableSegments;

TableSlotManager(Module& module);
TableSlotManager(Module& module,
const std::vector<std::unique_ptr<Module>>& secondaries);

Table* makeTable();
ElementSegment* makeElementSegment();
Expand Down Expand Up @@ -149,7 +151,9 @@ void TableSlotManager::addSlot(Name func, Slot slot) {
funcIndices.insert({func, slot});
}

TableSlotManager::TableSlotManager(Module& module) : module(module) {
TableSlotManager::TableSlotManager(
Module& module, const std::vector<std::unique_ptr<Module>>& secondaries)
: module(module), secondaries(secondaries) {
// If possible, just create a new table to manage all primary-to-secondary
// calls lazily. Do not re-use slots for functions that will already be in
// existing tables, since that is not correct in the face of table mutations.
Expand Down Expand Up @@ -233,8 +237,18 @@ TableSlotManager::TableSlotManager(Module& module) : module(module) {
}

Table* TableSlotManager::makeTable() {
return module.addTable(
Builder::makeTable(Names::getValidTableName(module, Name::fromInt(0))));
Name name = Names::getValidName("0", [&](Name test) {
if (module.getTableOrNull(test)) {
return false;
}
for (auto& secondary : secondaries) {
if (secondary->getTableOrNull(test)) {
return false;
}
}
Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we only create an active table once, no?

Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 57d63fc

return true;
});
return module.addTable(Builder::makeTable(name));
}

ElementSegment* TableSlotManager::makeElementSegment() {
Expand Down Expand Up @@ -347,7 +361,7 @@ struct ModuleSplitter {
void setupTablePatching();

ModuleSplitter(Module& primary, const Config& config)
: config(config), primary(primary), tableManager(primary),
: config(config), primary(primary), tableManager(primary, secondaries),
exportedPrimaryFuncs(initExportedPrimaryFuncs(primary)),
exportedPrimaryItems(initExportedPrimaryItems(primary)) {
classifyFunctions();
Expand Down
45 changes: 45 additions & 0 deletions test/lit/wasm-split/table-name-conflict.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
;; RUN: wasm-split %s -all -g -o1 %t.1.wasm -o2 %t.2.wasm --split-funcs=split
;; RUN: wasm-dis %t.1.wasm | filecheck %s --check-prefix PRIMARY
;; RUN: wasm-dis %t.2.wasm | filecheck %s --check-prefix SECONDARY

;; Regression test for a bug when an existing table, which is to be split to the
;; secondary module, has the name '0'. The newly created active table should
;; have a different name.

(module
(table $0 0 externref)
(export "split" (func $split))
(func $split
(table.set $0
(i32.const 0)
(ref.null extern)
)
)
)

;; PRIMARY: (module
;; PRIMARY-NEXT: (type $0 (func))
;; PRIMARY-NEXT: (import "placeholder.deferred" "0" (func $placeholder_0))
;; PRIMARY-NEXT: (table $0 1 funcref)
Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying that $0 is not the internal name of the table? Why are we emitting a name at all, then?

Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I understand. So the input file's table name is0:

 (table $0 0 externref)

And when it tries to make an active table, we start from 0, but because it collides with the existing secondary table, getValidName adds _0 to the name, so the active table's name will be 0_0.

But because the table's hasExternalName is not set, when we write the binary, that name is not separately written to the name section. And this test expectation is printed by wasm-dis. wasm-dis doesn't have any name information on tables, so it just blindly assign numbers from 0. That's why both tables, one in the primary (newly created active table) and secondary (existing externref table split out) are named 0.

As I said in #8708 (comment), we can make it preserved if debug option is enabled.

Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I was missing that there was a round-trip through the binary involved.

Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we can pass -S to wasm-split to have it write text modules, then we can cat them in the test to see the original names without the round trip.

Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 24a4d91

Copy link
Copy Markdown
Member

@tlively tlively May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the new table is still called $0?

Copy link
Copy Markdown
Member Author

@aheejin aheejin May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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)

;; PRIMARY-NEXT: (elem $0 (i32.const 0) $placeholder_0)
;; PRIMARY-NEXT: (export "split" (func $trampoline_split))
;; PRIMARY-NEXT: (export "table" (table $0))
;; PRIMARY-NEXT: (func $trampoline_split
;; PRIMARY-NEXT: (call_indirect (type $0)
;; PRIMARY-NEXT: (i32.const 0)
;; PRIMARY-NEXT: )
;; PRIMARY-NEXT: )
;; PRIMARY-NEXT: )

;; SECONDARY: (module
;; SECONDARY-NEXT: (type $0 (func))
;; SECONDARY-NEXT: (import "primary" "table" (table $timport$0 1 funcref))
;; SECONDARY-NEXT: (table $0 0 externref)
;; SECONDARY-NEXT: (elem $0 (table $timport$0) (i32.const 0) func $split)
;; SECONDARY-NEXT: (func $split
;; SECONDARY-NEXT: (table.set $0
;; SECONDARY-NEXT: (i32.const 0)
;; SECONDARY-NEXT: (ref.null noextern)
;; SECONDARY-NEXT: )
;; SECONDARY-NEXT: )
;; SECONDARY-NEXT: )