This lgtm, but fwiw I'm polishing a bunch of PRs I will send you shortly that are aimed at detecting all of the malformed cases in BinaryReader and Parser alone. The end result is the ability to (a) detect all of the "assert_malformed" tests without running the validator, and (2) roundtrip all of the "assert_invalid" tests through the text format (which has surfaced some small bugs in the reader/parser and printer).

I think this test case is one of the "malformed" cases (meaning that it should not require validation to detect -- it's a violation of the binary and text format syntaxes).

Oh nice! And thanks!

Do you have a branch with a WIP state I could poke around? I'm curious how you implemented it and if it required duplication of logic in the validator or if logic ended up being shifted around. The main reason in the past I didn't sync up the malformed/invalid tests is that it would require infrastructure that otherwise wasn't necessary but I never really thought too too deeply about it beyond that. For example I'm not sure if the infrastructure is extra or could be moved around, or if it were extra if it's even really all that costly.

This bug mostly seems to have come about as a direct result of me thinking that it's ok to defer the error here to the end, but that was unexpectedly subtle for consumers who consume an instruction-at-a-time and don't receive the error until the end. Only accepting syntactically valid modules though would indeed be nice and could remove some internals from the validator about checking for end-after-end or similar.

Sure, here is the WIP branch: https://github.com/keithw/wasm-tools/tree/malformed-invalid-separation

I tried to split it into a somewhat logical sequence of commits -- this is probably the sequence of PRs I would tentatively have been thinking to send, but happy to adapt to any feedback/thoughts. I think for the most part the logic got shifted around (e.g. from the validator to the operators reader).

Thanks! That looks pretty reasonable to me, but one of the takeaways is similar to this comment where I'd ideally prefer to keep validation isolated to validation instead of having the feature checks/etc happen elsewhere too. For example there are a number of new checks for features outside of the validator that are now present in the operator reader for things like flags on tables/memories, tags, data count processing, etc. Is that all required to be there in the sense that a test somewhere forced you to make that change?

Basically from a spec-perspective the change seems good, but from a code organization/maintainability perspective it's not the greatest change because validation is now smeared across more parts of the codebase and it's not clear, to me at least, what ends up in validation and what ends up in parsing validation.

Thank you for taking a look! To me there is a sort of logic to what ends up in the binary reader/parser vs. the validator. Basically:

• if something is malformed (a violation of the binary syntax), then the binary reader/parser should be able to detect that by itself, without needing to run the validator
• in every other case, we should be able to go successfully from binary -> text -> binary2 -> text2 by parsing and printing, and text and text2 should always be identical. And, if the original binary was something wasm-tools generated in a canonical form (because it came from the wat crate), then binary and binary2 should also be identical.

Almost all the code changes basically followed from those principles -- if there's an assert_malformed somewhere in the tests, then I had to make sure the binary reader/parser could detect the issue (but it also means the validator doesn't need to do that check or keep the required state for it). And if there was an assert_invalid, I had to make sure wasmprinter could print it in a way that roundtripped through the parser (even if it was totally invalid). E.g. a case like (module (func end)) can't really be represented in the binary format and roundtripped, so I think this kind of thing has to be considered malformed.

That makes sense yeah, and IIRC I tried to get this all working with wasmparser a long time ago and basically gave up. At a meta-level I personally found the distinction between malformed/invalid to be not useful from an engine perspective because at the end of the day an engine only cares if the module is valid or not, and if it's not valid it doesn't really care how it's invalid. In that sense I've historically avoided a change like this where, for example, the operator parser maintains a stack of block when the operator validator also maintains a stack of blocks, just slightly different. From a pure software engineering perspective that's duplication for no purpose, but from the spec perspective it's there for the exact malformed/invalid conditions.

I realize though that what I'm saying here basically amounts to "I think the spec is wrong" which is swimming upstream and generally a losing prospect. The objective cost of maintaining two stacks, for example, is quite low and the practical duplication is pretty minimal.

The other part that I've been pretty uncomfortable about is that wasm-tools has the job of "let's merge the spec and all proposals together" where upstream specs effectively never do that. All spec repos are simply points in time and don't refer to each other at all. That makes it pretty frustrating, for example, that in this example it has to be malformed and tested less compared to if it were invalid. To make matters worse all tests effectively need to change once proposals are merged to the spec. For example if/when shared-everything-threads is merged that test would presumably need to become assert_invalid as well?

Another example of this "juggling proposals" problem is that there's a few locations that you've added feature checks, such as rejecting parsing the tag section unless the exceptions proposal is enabled. What I find unfortunate about that is that this is ad-hoc (not your fault, I presume you're just following tests). For example simd instructions aren't gated in parsing by the simd feature, nor are instructions from any other proposal. The datacount section isn't gated by bulk-memory, etc.

Overall I personally like the spirit of distinguishing assert_{malformed,invalid} but I find that it falls down quickly in the face of proposals to change the spec. It forces wasmparser into a weird state where things are guaranteed feature-gated in the validator but some things are also feature-gated in binary parsing, and the binary parsing gating is basically entirely guided by "does someone happen to test this now-valid construct was invalid before".

That makes sense yeah, and IIRC I tried to get this all working with wasmparser a long time ago and basically gave up. At a meta-level I personally found the distinction between malformed/invalid to be not useful from an engine perspective because at the end of the day an engine only cares if the module is valid or not, and if it's not valid it doesn't really care how it's invalid.

I think you're 100% right that an engine doesn't have to care about any of this. It's probably relevant context that I'm hoping to use wasm-tools to power an IDE to teach freshman computer science in Wasm (an IDE that makes syntax errors impossible but guides the students visually to pass validation), so I have been caring a lot about this distinction recently and was hoping to have a clear story for the students. :-) And, I've been hoping to deprecate the WABT library (and port wasm2c to run on wasmparser); this seems like one of the last big pieces to get parity (in some respects -- of course wasm-tools is way ahead in many others).

In that sense I've historically avoided a change like this where, for example, the operator parser maintains a stack of block when the operator validator also maintains a stack of blocks, just slightly different. From a pure software engineering perspective that's duplication for no purpose, but from the spec perspective it's there for the exact malformed/invalid conditions.

Yes, understood.

The other part that I've been pretty uncomfortable about is that wasm-tools has the job of "let's merge the spec and all proposals together" where upstream specs effectively never do that. All spec repos are simply points in time and don't refer to each other at all. That makes it pretty frustrating, for example, that in this example it has to be malformed and tested less compared to if it were invalid. To make matters worse all tests effectively need to change once proposals are merged to the spec. For example if/when shared-everything-threads is merged that test would presumably need to become assert_invalid as well?

I share your frustration! (And yes, I think those flag values wouldn't be malformed anymore once shared-everything-threads is merged to the main branch of the spec.) This seems to be a "modern Web-style" pattern (it's not like they version HTML anymore either). :-/ I agreed with the sentiments in WebAssembly/spec#1788 but it doesn't seem trivial to solve.

BTW if it's any consolation, I don't think it's really "tested [much] less compared to if it were invalid" because in the eventual wast.rs, for an assert_malformed test, it's going to run the parser (by itself) to make sure it detects it, but then it's also (separately) going to run the validator to make sure that also fails (to make sure the validator is actually running the parser properly, e.g. calling finish at the end of a function).

Another example of this "juggling proposals" problem is that there's a few locations that you've added feature checks, such as rejecting parsing the tag section unless the exceptions proposal is enabled. What I find unfortunate about that is that this is ad-hoc (not your fault, I presume you're just following tests).

Ok that one we can get rid of -- it's self-inflicted by a single local test that requires even an empty tag section to be a failure unless exceptions are enabled (https://github.com/bytecodealliance/wasm-tools/blob/main/tests/cli/missing-features/missing-exceptions.wast#L3). If you're okay getting rid of that test we can get rid of the feature check on the tag section. (I just repushed with that change.)

For example simd instructions aren't gated in parsing by the simd feature, nor are instructions from any other proposal.

Yes, I think the spec was developed with the expectation that new opcodes would be added, so the spec tests didn't have negative tests for unrecognized opcodes afaik...

The datacount section isn't gated by bulk-memory, etc.

Agreed -- I just got rid of the check on the tag section (and the corresponding test) so at least it's more consistent now.

Overall I personally like the spirit of distinguishing assert_{malformed,invalid} but I find that it falls down quickly in the face of proposals to change the spec. It forces wasmparser into a weird state where things are guaranteed feature-gated in the validator but some things are also feature-gated in binary parsing, and the binary parsing gating is basically entirely guided by "does someone happen to test this now-valid construct was invalid before".

Yes, it's true, but at least I don't think there's anything being redundantly checked in the parser vs. in the validator. Some features did enlarge the binary syntax, and some features enlarge what's considered valid, so I don't think it's crazy that both the parser and the validator have to check the enabled features at different points. But hopefully never for the exact same thing.

I'm just about to make the first "big" PR (with probably two more after that) and hopefully you find it... mostly okay? Thank you for your hyperspeed reviews on everything of course and I do share much of these sentiments...

Ok after reading over #2134 I'm left with a few conclusions:

• That feels like a much better way to solve the original bug here, the panic in Wasmtime, than this PR.
• We should expect some continued degree of "pain" juggling spec tests and various proposals. I don't think wasmparser: detect "malformed" cases in parser alone (without validator) #2134 makes things measurably worse in this respect.
• Overall it feels better to me to match the binary syntax of the spec in terms of "reading will fail unless the syntax is met".

Given all that I'm going to close this in favor of #2134 which, when merged, should fix the original panic in Wasmtime as well (once this update is integrated).

Thanks again @keithw for your work here and discussion with me, it's very much appreciated!