Write memory.init operands in the order the text format expects - #2838
Open
Nishuuzz wants to merge 1 commit into
Open
Write memory.init operands in the order the text format expects#2838Nishuuzz wants to merge 1 commit into
Nishuuzz wants to merge 1 commit into
Conversation
Collaborator
|
Good catch! Please update the failed test |
The text format names the memory before the data segment:
(memory.init $mem $data ...)
ParseMemoryInstrVar() reads them that way, but the writer emitted the
data segment first and the memory second, which is the operand order of
the binary encoding. So wasm2wat produced text that wat2wasm could not
read back:
$ wasm2wat --enable-multi-memory m.wasm -o m.wat
$ wat2wasm --enable-multi-memory m.wat
m.wat:14:19: error: data_segment variable out of range: 1 (max 1)
memory.init 0 1
^
The two indices are only distinguishable when the memory index is not
zero, so this needs multi-memory to show up. With a single memory the
writer omits the memory index and the remaining operand is the data
segment, which parses correctly, which is why it went unnoticed.
Where the indices happen to both be in range the text still parses, but
it means the wrong thing: the memory and data segment come back swapped.
table.init already writes its table index before its segment index, and
its parser documents the swap, so this brings memory.init in line with
its sibling.
Found by running the spec testsuite through wasm2wat and back: four
modules in proposals/multi-memory failed to reparse, and all four are
fixed by this.
Nishuuzz
force-pushed
the
memory-init-operand-order
branch
from
August 26, 2026 08:34
66790cc to
8450933
Compare
Contributor
Author
|
Thanks — that failure was my own test, not a pre-existing one. #2837 turned multi-memory on by default and renamed the flag to Rebased on main and dropped the flag. CI is green now. The writer change itself is unchanged and rebased cleanly. I re-checked the test still earns its place on the new base: with wat-writer.cc reverted to main it fails with the original error, and passes with the fix. Roundtrip suite is at 96 passing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The text format names the memory before the data segment:
ParseMemoryInstrVar()reads them in that order, but the writer emitted the data segment first and the memory second, which is the operand order of the binary encoding. Sowasm2watproduced text thatwat2wasmthen refused:The two indices are only distinguishable when the memory index isn't zero, so it takes multi-memory to show up. With a single memory the writer leaves the memory index out and the one remaining operand is the data segment, which parses fine — which is presumably why nobody hit this. When both indices do happen to be in range the text still parses, but it means the wrong thing, with the memory and data segment swapped.
table.initalready writes its table index before its segment index, and its parser has a comment spelling out the swap, so this just bringsmemory.initin line with its sibling.How I found it: ran every module in the spec testsuite through
wasm2watand back (3542 modules thatwasm-validateaccepts). Four modules inproposals/multi-memoryfailed to reparse, all with the error above, and all four are fixed by this. The same sweep also turns up 48 modules inproposals/function-referencesfailing withmissing table initializeron(table (;0;) 10 (ref func))— that's a separate problem in the table writer and I haven't touched it here.The new roundtrip test is the memory equivalent of the existing
elem-nonzero-table.txt. It fails before this change (wat2wasm rejects the round-tripped text, exit 2) and passes after. The whole roundtrip suite is green, 95 tests.One thing I should flag about my local testing: the wasm2c tests don't run on this machine because the harness can't find a working C compiler, so 444 of them fail here regardless of this change. Of the remaining failures,
wat2wasm_stdout.txt,regress-2034andregress-2039fail identically with the writer reverted to main, so they're pre-existing and unrelated. CI will cover what I couldn't.