Keep code metadata annotations attached when folding - #2833
Conversation
|
It sounds like you are saying that code annotation are fundamentally incompatible with the folded wat format, is that right? If so, I wonder if this is something that we should fix in the spec? Perhaps you could file an upstream issue in the spec repo just to check that this is indeed a known issue and not something that we should fix? Can you also confirm what binaryen does in this case? I know it likes to output the folded form. |
|
I believe we (now) have round-trippable annotations in the folded format in wasm-tools (recent bugfix here: bytecodealliance/wasm-tools#2578), so I know it's possible to do this. :-) I think the upstream spec is okay. |
|
@keithw, how does it handle the above example? |
|
Well, good question. I'm realizing that the wast crate ignores annotations other than (module
(func $f (param i32) (result i32)
i32.const 1234
local.get 0
(@metadata.code.branch_hint "\01") i32.add
return))(well-formed but invalid because a branch hint can't go on "i32.add"), wasm-tools prints it folded as: (module
(type (;0;) (func (param i32) (result i32)))
(func $f (;0;) (type 0) (param i32) (result i32)
(return
(@metadata.code.branch_hint "\01")
(i32.add
(i32.const 1234)
(local.get 0)))
)
)and then parses that back to flat format as: (module
(type (;0;) (func (param i32) (result i32)))
(func $f (;0;) (type 0) (param i32) (result i32)
i32.const 1234
local.get 0
(@metadata.code.branch_hint "\01")
i32.add
return
)
) |
|
@Nishuuzz do you think that form looks correct? Could we find a way to do this rather than give up on folding: |
|
Yes, that form is right — and I was wrong to describe this as the text format not allowing it. It's wabt's parser that doesn't, which is a different thing, and my description overstated it. The rule that makes it work is that the annotation binds to the operator of the folded expression that follows it, rather than to the first instruction that expression expands to. So in @keithw's example the annotation sits before On binaryen: it does handle these, and it prints the same shape. From (block $out
(@metadata.code.branch_hint "\00")
(br_if $out
(local.get $x)
)
(@metadata.code.branch_hint "\01")
(br_if $out
(local.get $x)
)
)So binaryen and wasm-tools agree on the convention and wabt is the odd one out. That being the case I don't think there's anything to file upstream. wabt rejects it on the way in today: so this needs a parser change as well as a writer one. In Happy to do it that way rather than land the de-folding version, so I'll rework this branch instead. |
A code metadata annotation has arity {0, 0}, so the folded writer treated
it as an instruction. That wrapped it in its own parentheses, and because
it produces no results it also flushed the operands of the instruction it
was meant to decorate:
((@metadata.code.test "aa\01a"))
(return
(i32.add))
The result could not be parsed back, and i32.add had lost both operands.
Branch hints are code metadata too, so they broke the same way.
Annotations are now held until the instruction they decorate is pushed
and carried on its tree node, so they are written immediately before that
instruction's folded expression:
(return
(@metadata.code.test "aa\01a") (i32.add
(i32.const 1234)
(local.get 0)))
which is the form binaryen and wasm-tools both emit and round-trip.
The parser did not accept an annotation there, so it learns to. An
annotation decorates the operator of the expression that follows it, and
a folded expression expands to its operands before that operator, so the
annotation is spliced between the two. This has to happen both in an
operand list and at statement level, where block-like instructions such
as the if a branch hint decorates are written, so both paths share
AppendAnnotatedInstr.
d8b4269 to
9167864
Compare
3 participants
wasm2wat --fold-exprsproduces output it cannot read back for any module carrying code metadata, and corrupts the instruction the metadata belongs to on the way.For
test/dump/code-metadata.txt, where the annotation decoratesi32.add:The annotation has been wrapped in parentheses of its own, which does not parse —
error: unexpected token "metadata.code.test", expected an instr.— andi32.addhas lost both operands. Branch hints are code metadata too, sotest/parse/branch-hints.txtbreaks the same way.The cause is that a
CodeMetadataExprhas arity{0, 0}, soWriteFoldedExprhands it toPushExprlike an instruction. That gives it its own node, hence the parentheses, and sinceresult_count == 0it also flushes the expression stack, which detaches the operands of the instruction it was supposed to decorate.The output form
Annotations are now held until the instruction they decorate is pushed and carried on that instruction's tree node, so they are written immediately before its folded expression:
This is what binaryen and wasm-tools both emit and round-trip, as @keithw showed above;
test/lit/branch-hinting.wastin binaryen has the same shape withRTRIPchecks over it. Thanks to both of you for pushing back on the first version of this, which de-folded the annotated instruction instead. I had described the flat form as a limitation of the text format, and that was wrong — it is a limitation of wabt's parser, which is a different thing and fixable.The parser change
wabt rejected an annotation in that position, so it learns to accept one. The rule is that an annotation decorates the operator of the expression that follows it, and a folded expression expands to its operands before that operator, so the annotation is spliced between the two. Get that wrong and the output still parses but the offset recorded in the metadata section moves to a different instruction, which is the quiet failure rather than the loud one.
It has to happen in two places: in an operand list, and at statement level, where block-like instructions are written. The second is the case branch hints actually hit, since they decorate an
if. Both paths shareAppendAnnotatedInstr.Testing
test/roundtrip/fold-code-metadata.txtcovers an annotation on an instruction with operands, one on an instruction without, and one on anif, that last being the branch-hint shape.run-roundtripre-assembles and compares against the original binary, so it catches a moved offset as well as a parse failure.Both reproducers above now round-trip to byte-identical binaries.
I ran every module in
test/throughwasm2watin each of its writer variants — default,--fold-exprs,--inline-exports,--inline-imports,--generate-names,--no-debug-names— and assembled each result again.--fold-exprswas 1102 of 1108 before this change and is 1108 now, with the other five unchanged.Suites:
roundtrip95,dump148,parse758,typecheck119,desugar6, no failures, and unit tests 135. No existing expectation changed.