1253572 - Baldr: Update binary encoding to add arity immediates for call/return/branch

Status: RESOLVED DUPLICATE of bug 1263202

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Test case:

(module
  (func
    (return (i32.const 1))
  )
)

As the function returns void, the way we type-check in Wasm.cpp makes it that we skip emitting the i32.const. So the decoder thinks it is at the end of the function's section just after the return (because the function expr list is of size 1), but it isn't because of the (i32.const 1), and thus the error message shown is "function section byte size mismatch".

There should be a better error message here, directly pointing to the return (mismatchError("i32", "void")).

However, when we're decoding the Void return, we can't distinguish between the above test case and:

(module
  (func
    (return)
    (i32.const 1)
  )
)

Unless we require void return to always contain a value (a nop, in the worst case?), so that all void returns can have a value attached to them (and hence a Void subexpression), I have no idea how we can emit the error at the right time here.

It would be cool that a void return could have a Void subexpression, that being said. C++ allows that. Thinking about it, the only advantage of this in terms of packing would be to be able to spare 1 expr in an expr-list, so not sure it is worth it. Thoughts?

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Note it doesn't only affect this particular error message as shown in comment 0, e.g.:

(module
  (func
    (block
      (i32.add
        (i32.const 0)
        (return (br 0))
      )
    )
    (return)
  )
)

This one shows "branch depth exceeds current nesting level", because the decoder already has left the block when it sees the (br 0).

Comment 2

[Luke Wagner [:luke]]

Ah hah, this is another instance of the whole void-return-type issue.  So technically we're doing the right thing here which is failing validation.  You could argue that wasmTextToBinary() should throw a syntax error, but I kindof like keeping it lose so that invalid binaries can be generated (and it's not semantically visible).

The change forthcoming (post-demo, though) is to give return/br/br_if an explicit arity immediate so without any context the decoder knows whether to recurse or not.  With that, the (return (i32.const 1)) in your first example would succeed because the expected type is void which accepts anything.

So given all that, the "fix" to this bug is adding arities to BinaryEncoding.md and then SM, so I'll rename.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Fixed in bug 1263202.