Write init expressions folded so they can be parsed back - #2830
Conversation
WriteInitExpr wrapped the whole expression list in a single pair of
parentheses and then wrote the instructions unfolded. That is fine for
the usual single-instruction init expression, but an extended constant
expression has several instructions, and the result cannot be parsed:
(global (;1;) (mut i32) (i32.const 44
i32.const 3
i32.sub))
wasm2wat produced this for a module using the extended-const proposal, so
its output no longer assembled. Globals, and data and elem offsets, were
all affected, and --fold-exprs made no difference because init
expressions never went through the folded writer.
Write them with WriteFoldedExprList instead, which emits a single folded
expression that is valid in all of those positions:
(global (;1;) (mut i32) (i32.sub
(i32.const 44)
(i32.const 3)))
Single-instruction init expressions are unchanged.
run-roundtrip.py did not accept --enable-extended-const, which is why
this was never covered; add the flag along with a roundtrip test.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
LGTM, but is there no way to use the flat (non-folded) form with extended const expressions? i.e. do we need to force one form over the other here or can we support both?
|
Yes, flat is possible, but not uniformly, and that's what pushed me towards folding. Globals take a bare instruction sequence, so So keeping the flat form means teaching the writer to emit Happy to do it the other way if you'd rather the writer respect |
|
This approach seems like a good improvement as is. |
`wasm2wat --generate-names` produces output it can't read back when a module has an export with an empty name. ```wat (module (func $f) (export "" (func $f))) ``` Disassembled with `--generate-names`: ```wat (func $ (type $t0)) (export "" (func $)) ``` and assembling that back gives `error: empty identifier.` Generated names are derived from import and export names, built as `"$"` followed by the name, so an empty export name leaves just the sigil. This skips an empty name instead, and the item keeps whatever name the rest of the pass gives it — another of its export names if it has one, otherwise the index-based name it would have had anyway. Empty export names are legal and do turn up: `test/wasm2c/export-names.txt` already has one, which is where I hit this. Imports go through the same helper but can't reach it, because the name there is always `module_name + "." + field_name` and so is never empty. I put the check in the shared helper anyway rather than in the export path, since it's guarding a property of the generated identifier rather than anything specific to exports. ### How I found it Same round-trip sweep as #2830, extended to the writer variants rather than just the default one: for every module in `test/`, disassemble with `--fold-exprs`, `--inline-exports`, `--inline-imports`, `--generate-names` and `--no-debug-names` in turn, then assemble the result again. `--generate-names` was 1105 of 1106, and this was the one failure. It is 1106 now. ### Testing `test/roundtrip/generate-empty-export-name.txt` covers a func whose only export name is empty, alongside one with a usable export name so both paths are visible in the output. It fails without the change with the `empty identifier` error above. Note it deliberately does not pass `--debug-names`: with a name section present the original names survive and the generator never runs, so the test wouldn't exercise this at all. `roundtrip` goes from 93 to 94, and `desugar`, `typecheck`, `parse` and the unit tests are unchanged. ### Unrelated, while I was in there The same sweep found `--fold-exprs` output failing to re-assemble for code metadata annotations and for branch hints: ``` test/dump/code-metadata.txt error: unexpected token "metadata.code.test", expected an instr. test/parse/branch-hints.txt error: unexpected token (, expected ). ``` That looks like a separate problem in how annotations are placed in folded output, so I've left it out of this PR. Happy to open an issue for it if it isn't already known.
2 participants
wasm2watproduces output it can't read back for modules that use the extended-const proposal.An init expression with more than one instruction comes out like this:
which fails to assemble:
WriteInitExpropens a single pair of parentheses around the whole expression list and then writes the instructions unfolded. For the ordinary one-instruction case that gives(i32.const 45)and is fine, but with several instructions the leading(reads as the start of a folded expression and everything after it is a syntax error. Globals, data offsets and elem offsets are all affected.--fold-exprsdoesn't help, because init expressions never went through the folded writer at all.This writes them with
WriteFoldedExprListinstead, which emits one folded expression, valid in every position an init expression can appear:Single-instruction init expressions are unaffected — those still print as
(i32.const 45), so existing expectations don't move.How I found it
I ran every
.txtundertest/through wat2wasm → wasm2wat → wat2wasm with--enable-alland compared the two binaries. 1102 modules round-tripped byte-identically and three failed to re-assemble, all for this reason:test/dump/extended-const.txt,test/dump/invalid-data-segment-offset.txtandtest/parse/module/bad-global-invalid-expr.txt. After the change all 1105 round-trip and the binaries match.test/dump/extended-const.txtalready covers this module, but it's an objdump test, so nothing ever fed the disassembly back to the assembler.Testing
Added
test/roundtrip/extended-const.txt, covering a global, a data offset and an elem offset with extended constant expressions. It fails without the writer change with the parse error above, and passes with it.run-roundtrip.pyhad no--enable-extended-const, which is the reason the roundtrip suite couldn't have caught this in the first place, so I added the flag and passed it to both tools alongside the existing ones.test/roundtripgoes from 92 to 93 passing, and the unit tests are unchanged at 135.One caveat on my local runs: this is Windows, and a lot of the wider suite can't execute here — Application Control blocks the freshly built tools for several directories, and the wasm2c tests need a
cl.exeI don't have.roundtrip,desugarandtypecheckrun clean, and I checked that no expected output anywhere intest/contains the old unparseable shape, but I'd rather flag that than imply I ran everything.