wasm-as/wasm-dis seem to miscompile ref.null with type-index heap type

[ddegazio]

I've noticed some seemingly incorrect behavior in binaryen working with one of the cases from the br_if.wast spec test on the wasm-3.0 branch:

(module
  (type $t (func))
  (func $f (param (ref null $t)) (result funcref) (local.get 0))
  (func (result funcref)
    (ref.null $t)
    (i32.const 0)
    (br_if 0)  ;; only leaves funcref on the stack
    (call $f)
  )
)

(see WebAssembly/gc#516 for discussion about this issue).

Using wasm-as version 118 and building this with --enable-reference-types and --enable-gc, I notice:

1. In the assembled binary, ref.null $t becomes d0 73, which corresponds to (ref.null nofunc) instead of (ref.null $t).
2. This test is expected to fail to validate, but wasm-as doesn't report a validation failure.

Likewise, disassembling the assembled binary from the spec tests:

\x00\x61\x73\x6d\x01\x00\x00\x00\x01\x8e\x80\x80\x80\x00\x03\x60\x00\x00\x60\x01\x63\x00\x01\x70\x60\x00\x01\x70\x03\x83\x80\x80\x80\x00\x02\x01\x02\x0a\x99\x80\x80\x80\x00\x02\x84\x80\x80\x80\x00\x00\x20\x00\x0b\x8a\x80\x80\x80\x00\x00\xd0\x00\x41\x00\x0d\x00\x10\x00\x0b

(I typically use echo -ne "\x00\x61\x73\x6d..." to convert this to binary)

...wasm-dis disassembles the ref.null in this binary, encoded as d0 00, as (ref.null nofunc). Instead, I think heap type 00 should be interpreted as a type index.

[tlively]

The lack of validation failure is because we do not faithfully implement the proper validation of branches as discussed in WebAssembly/gc#516. To avoid emitting invalid binaries, we instead have a fixup that inserts extra casts as necessary when emitting a binary: e5f2edf

The behavior where the ref.nulls type annotations are changing is a separate issue. In a valid module, it is always valid to replace ref.null $t with ref.null bot where bot is the bottom heap type for whatever hierarchy $t is in. Doing that replacement also makes the type more precise, e.g. (ref null none) instead of (ref null $t), so it's better for optimization. Because of that, we maintain an invariant that we only store bottom heap heaps on RefNull expressions. This does mean we currently accept invalid modules that should be invalid due to the original types of the ref.null instructions.

Now that we are trying to run the upstream spec tests in Binaryen, issues like this are going to become more important to fix. I'm thinking the best course of action will be to perform real Wasm validation at parse time, then subsequently use the current, looser validation for the IR. @kripken, does building validation into IRBuilder sound reasonable to you?

[tlively]

Even with proper validation, we will not precisely round-trip ref.null with type indices. In general, perfect round-tripping like that is a non-goal for Binaryen.

[kripken]

does building validation into IRBuilder sound reasonable to you?

Do you mean just validation of the few things we have to do there, that can't be done on the IR? That sgtm, and is what we do already with some validation that we have in the binary reader for example.

[ddegazio]

Even with proper validation, we will not precisely round-trip ref.null with type indices. In general, perfect round-tripping like that is a non-goal for Binaryen.

I see. To me it was definitely surprising behavior for wasm-as, is the expectation that clients who want precise assembling should just use WABT? Up-front validation would certainly cover this specific case, but maybe in cases where the module is valid and still doesn't round-trip exactly, there could be an optional warning or strictness setting to indicate when the module has been changed.

[tlively]

is the expectation that clients who want precise assembling should just use WABT?

The more actively maintained option for assembling and disassembling would be https://github.com/bytecodealliance/wasm-tools, but WABT also works well as long as you don't need GC support.

A warning or error is an interesting idea, but my gut reaction is that the benefit wouldn't be worth the complexity. There is rarely a need for precise assembly/disassembly, and when there is, I expect users to quickly discover that Binaryen isn't the tool for the job, as you have.

does building validation into IRBuilder sound reasonable to you?

Do you mean just validation of the few things we have to do there, that can't be done on the IR? That sgtm, and is what we do already with some validation that we have in the binary reader for example.

No, to get full fidelity we would need a more heavyweight approach than we've taken so far. To validate unreachable code, branches, and nulls properly, we would need to maintain a separate type stack and implement the full algorithm for standard validation. Thankfully this could be encapsulated entirely in IRBuilder. We could add an option to turn it off to continue testing IR validation errors.

[kripken]

No, to get full fidelity we would need a more heavyweight approach than we've taken so far.

How much work do you think that would be? If it's significant then I'm not sure getting a few more spec tests to pass is worth it.

[tlively]

I don't think it would be too significant. It would basically be just one more Visitor with a small amount of custom logic for each expression type, plus a tiny amount of shared infrastructure for manipulating the stack. This isn't something I would want to do until it's the last thing blocking us from running spec tests, though.

I think the marginal benefit of getting those last few spec tests running will be large, though. Beyond getting to run all the other tests in files we would otherwise have to skip because of one or two bad test cases, we would also have an in-tree way of validating that the binaries we emit are valid for real. Currently we rely on fuzzing with V8 to detect problems there. It will be much better not to have to rely on V8 for that.

[kripken]

Good points. But I think we can disable the one or two bad testcases in a file without ignoring the entire file? We can identify them by line or by module index (or name, if the modules are named). In fact I seem to recall we had a mechanism for just that, but I could be wrong.

I 100% agree it is important to have tests that we emit valid binaries. But tests that check we don't accept invalid ones seem more like a nice-to-have.