wasm-dis should run validation

[martianboy]

We recently allowed printing expression contents with respect to features in the Print pass (#3537) and we needed that as part of enabling multi-table modules in the Binaryen IR (#3517). But after I updated the tests, I noticed there's a difference between .from-wast and .fromBinary test files, in that .from-wast files were generated correctly with the correct features, but .fromBinary files were always generated with all features disabled.

Now, for .fromBinary tests we use wasm-as to compile a .wasm binary, and then use wasm-dis to check the text output. But since wasm-dis does not have the feature options, the only way it can print with features is if the module has a features section, wasm-as does not have an option to emit.

Two solutions are possible in my view:

1. Add feature options to wasm-dis, so we have the flexibility to print with the desired features.
2. Allow tools like wasm-as and wasm-split to emit a features section, so the features are read and applied while parsing the binary in wasm-dis.

[tlively]

Option 1 would probably be the least surprising for users.

[kripken]

Does wasm-dis not assume all features are present, so that it can always print a module? If so, isn't it an option to ignore the difference between the from-text and from-binary results in the test suite? I agree the difference is a little odd, but if it's just in the test suite, and it's not causing a risk of us missing actual bugs, I'm not that worried.

[martianboy]

@kripken No wasm-dis is not assuming all features. In fact it does not assume any features unless a features section is present in the binary. Otherwise there wouldn't be a difference between .from-wast and .fromBinary tests at all.

[kripken]

Oh, you're right @martianboy ... what confused me is that wasm-dis works on a file using features but without a features section. It works because we don't run the validator...

In that case, I think we should enable all features in wasm-dis, as that seems simplest?

[martianboy]

@kripken Yeah sure that works. I made PR #3548 that adds feature options, but if you think that's overkill, I can simply apply all features to the module right after parsing.

[kripken]

I think it would be better to enable them all. My mental model is that wasm-dis can disassemble any input module, no matter the features, so it should "just work".

@tlively what do you think?

[kripken]

@tlively you merged the PR that closed this. Personally I'd prefer the other option, but I don't feel strongly.

But as mentioned above I think that PR plus fixing validation in wasm-dis will lead to a bad result. That is, right now wasm-dis can disassembly anything because it does not validate. If we make it validate, then people will need to start to write wasm-dis -all on modules without a feature section - which is most modules in the wild. Or am I missing something?

[tlively]

Oops, sorry about that. I hadn't seen your most recent comment. I agree that wasm-dis should be able to disassemble any module by default, regardless of the features used. My mental model for the feature flags was that they control how the output is printed in cases like call_indirect, not that they should limit what modules wasm-dis accepts as input. I think we can avoid the bad outcome you described by continuing to have wasm-dis not run validation. What do you think about that solution? Is it important that we start validating in wasm-dis?

[kripken]

wasm-dis not validating is a bug, I think :smile: I guess we could redefine it as a feature (/workaround), but I suspect it will hurt us eventually when we are surprised to not get a validation error.

I guess we can leave it alone for now, as the bug is quite old and has not been noticed until now. I opened https://github.com/WebAssembly/binaryen/pull/3556 to at least document the issue in the code.