[interp] Fix operand types when tracing table.set - #2825
Conversation
sbc100
changed the title
| } | ||
| } | ||
| // table.set stores a reference whose type comes from the table, so opcode.def | ||
| // leaves that top operand's param type Void. The loop above then stops at the |
There was a problem hiding this comment.
Should we instead update the opcode.def? I'm not sure if there is a some non-void type that we could put there that would make more sense? Something that signifies a polymorphic type? Or is that what Void means?
There was a problem hiding this comment.
Yes, opcode.def is the better place, done. Void does double duty in that table: trailing Voids mean the operand doesn't exist, while an interior Void means it exists but its type isn't statically known. table.grow and table.fill already rely on the second meaning and Pick resolves it from the table. table.set is the one opcode whose polymorphic operand comes last, so its Void got stripped as a missing operand. I've switched that slot to Type::Any, which type.h already reserves for this sort of thing, and taught Pick to resolve Any the same way as an interior Void, so the special case is gone. The only other readers of the param columns are the type checker's memory ops and the c-writer's binary ops, so nothing else picks up the change. Full suite still passes.
Instead of special-casing table.set in TraceSource::Pick, record its polymorphic value operand in opcode.def using Type::Any. Trailing Void params mean the operand doesn't exist, so Pick's operand-count estimate stripped table.set's value operand and swapped the operand types. Any marks an operand that exists but whose type isn't statically known, and Pick resolves it from the table like an interior Void.
2 participants
wasm-interp --trace, on a valid module that stores through a funcref table:
table.set has two operands, an i32 index and a reference value, but opcode.def leaves the value operand Void because its type is taken from the table. Pick estimates the operand count by stripping trailing Void params, so it counts one instead of two and the operand-to-type mapping slips by one: the i32 index is handed the table element type and read back through Value::Get(). Under NDEBUG the assert is gone but the trace still prints the index as a bogus funcref.
Set table.set to two operands so the existing GetTableElementType fallback lands on the value operand and the index keeps its i32 type. table.get, table.grow and table.fill already map correctly.
Repro: