Update binary encoding for block, loop, and if signatures

[dschuff]

As noted in the individual commits. There's still some kind of problem with autoblock types/breaks which target the function implicit block, so not merging yet

[dschuff]

@kripken It's close but just wanted to get it up since I may not work on it more this weekend. Feel free to mess with it if you like.

[dschuff]

OK, so I figured out more about the issues. This code is consistent with the current old-style type-checking scheme, and it works with one exception, the following test from test/spec/return.wast:

  (func (export "as-br-value") (result i32)
    (block i32 (br 0 (return (i32.const 9))))
  )

The .s parser ignores the block signature, and the result in the IR is the following:

  (func $0 (type $0) (result i32)
    [unreachable] [unreachable] (block $block0
      [unreachable] (br $block0
        [unreachable] (return
          [i32] (i32.const 9)
        )
      )
    )
  )

Note that it considers the block to be unreachable rather than i32. This typechecks OK. Note also that without a block signature we don't know a priori whether the br should yield a value. The reason it can successfully parse the wast is because it has the ) that terminates the Element. In the binary format this was previously encoded in the br's arity immediate but now that's gone. With block signatures it's not necessary because the break target's arity tells how how many values to pop from the stack. When writing the binary, we write out the blocks with signatures, as the inferred type. But in this test the signature can't be inferred; the block end is unreachable, and the br targeting it is also unreachable.

I made a hacky workaround which, for the purposes of encoding, infers the arities of unreachable break targets and encodes those as i32 (which on the decoder side will be ignored other than for the purposes of decoding). But I think we won't be quite compatible with other binary consumers until we correctly get the types of those blocks (which I think also implies updating the type checking).

[kripken]

Interesting, I see. Ok, it does look like we need to fix that type checking. I can look into that, I opened the type-check-block-sigs branch for that, initial work shows it shouldn't be too hard. Most of the work was updating tests so far.

Meanwhile this looks good to land to me, once the type checking is fixed we can remove the hack.

[ghost]

Interesting example, but can't binaryen just note the signatures for blocks when it either parses them from the wast or decodes them from the binary, then it does not have to infer the block types, rather just check that all is valid. Their might be input sources for which the block signatures are not given, but it seem only necessary on those paths to infer the types, and these paths might just have to do some extra work to ensure that the fill block signatures are consistent top-down and bottom-up.

[kripken]

@dschuff: Ok, in that branch I have things mostly working with the new type checking. Spec tests all pass. One problem though is that the binary format tests don't work, perhaps related to the hack implemented in this PR? That branch is based off this one, and doesn't alter the hack - is that expected to work?

If you have a minute to look at it, build that branch, copy the main module in test/spec/br.wast into a file, then run wasm-as and wasm-dis on it, then wasm-shell on that output, and it gives some errors that it didn't give on the original file before binary.

[dschuff]

@JSStats yes, that's what I'm saying we should do (i.e. actually parse the block signatures and use them when building the IR). Currently we still have the same type-inference code from binary 0xb before block signatures were adopted, and we should fix that.

@kripken Currently (i.e. in this branch) the wast parser still ignores the block signatures entirely and the binary parser only uses them for decoding (the old type inference is still used when the block is finalized). I'll take a look at your branch, although I think we can probably merge this one as an intermediate step, and then put yours onto master.

[kripken]

@dschuff: yeah, sounds good to me to merge this. we can figure out the stuff in #717 later.