Repository navigation
wasm-dis should run validation #3545
Description
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:
- Add feature options to
wasm-dis, so we have the flexibility to print with the desired features. - Allow tools like
wasm-asandwasm-splitto emit a features section, so the features are read and applied while parsing the binary inwasm-dis.
Activity
tlively commentedon Feb 4, 2021
Option 1 would probably be the least surprising for users.
Add feature options to wasm-dis
kripken commentedon Feb 5, 2021
Does
wasm-disnot 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 commentedon Feb 5, 2021
@kripken No
wasm-disis 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 commentedon Feb 5, 2021
Oh, you're right @martianboy ... what confused me is that
wasm-disworks 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 commentedon Feb 6, 2021
@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 commentedon Feb 8, 2021
I think it would be better to enable them all. My mental model is that
wasm-discan disassemble any input module, no matter the features, so it should "just work".@tlively what do you think?
Add feature options to wasm-dis (#3548)
kripken commentedon Feb 8, 2021
@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-diswill lead to a bad result. That is, right nowwasm-discan disassembly anything because it does not validate. If we make it validate, then people will need to start to writewasm-dis -allon modules without a feature section - which is most modules in the wild. Or am I missing something?tlively commentedon Feb 8, 2021
Oops, sorry about that. I hadn't seen your most recent comment. I agree that
wasm-disshould 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 likecall_indirect, not that they should limit what moduleswasm-disaccepts as input. I think we can avoid the bad outcome you described by continuing to havewasm-disnot run validation. What do you think about that solution? Is it important that we start validating inwasm-dis?kripken commentedon Feb 8, 2021
wasm-disnot validating is a bug, I think 😄 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 #3556 to at least document the issue in the code.
[-].fromBinary tests are always executed with MVP features[/-][+]wasm-dis should run validation[/+]