[wasmparser] add atomic validator feature

[keithw]

This PR adds an atomic feature to the validator, exposing a FuncValidator::atomic_op function that tries to validate an operator and leaves the validator unchanged if the operator is invalid.

The feature adds a "rollback log" to the OperatorValidator; the log keeps track of any information destroyed during validation of the operator, so that the validator can be rolled back to its initial state if the operator turns out to be invalid. I tried to make this basically performance-neutral for anybody who doesn't need this. The rollback log is only present if the atomic feature is enabled, and it's only used if atomic_op is being used (i.e. the visit functions and FuncValidator::op don't use it).

We're using this as part of a Wasm development/teaching environment where we'd like the validator to be in a predictable state after an invalid operator.

[alexcrichton]

If performance is ok to compromise on a bit here, the Clone-ability of a FunctionValidator might be sufficient here because the validator could be cloned before the operation and then thrown away if validation succeeded. That'd likely have some quadratic performance in terms of stack depth, however, so I can see how that wouldn't be desirable for handling large functions.

[keithw]

Yeah, we are validating basically on every keystroke, so having to duplicate the operand/control/init stacks on every operator (valid or not) is probably too expensive for us.

[alexcrichton]

Would you be up for doing some quick measurements with the scale of wasm files you're expecting to see what it's like? I'd naively expect keystrokes to be dozens-of-milliseconds apart which is probably more than enough time to clone the validator and such, so my hunch is that it's probably not that noticable but if you're dealing with clang.wasm it might still be noticable.

The reason I ask is that this is pretty invasive in the validator in terms of hooking into the critical points of dealing with all the internal validator state. It's not unreasonable per-se and I'd agree that a compile-time feature is the way to go here, but even with that this is something that may not be that easy to maintain over time. Given that I'd like to try to push on the clone-ability of the validator (which in contrast should be pretty easy to maintain) and see if that works. If it doesn't work though it doesn't work, and this seems reasonable enough.

[keithw]

Sure, here's a (very) rough measurement... Let's say the largest file we'd plan to edit is around 100,000 lines. (The clang.wasm that used to be a w2c2 example is >10M lines.) The worst case is probably something like "a single function with 100,000 lines of i32.const 0". Running wasm-tools validate on an AMD Ryzen 7 PRO 4750U @ ~3.9 GHz:

• As-is, this takes around 1-3 ms to validate and report the error at the end.

• With an OperatorsReader and running FuncValidator::op on every Operator, this takes around 2-5 ms to validate and report the error.

• With the atomic feature, using an OperatorsReader and running FuncValidator::atomic_op on every Operator, this takes around 3-7 ms to validate and report the error.

• With an approach that clones the OperatorValidator on every operator, this is taking about 850-1000 ms.

(Edit to add: These numbers are for a release build running on native Linux x86-64; the real application is going to be running in Wasm in a browser which I think also makes us a little more concerned about possible performance gotchas.)

(Edit 2: made the numbers clearer and made the atomic benchmark actually use the new atomic_op function.)

[alexcrichton]

Hm well yes to a certain extent I understand that cloning has quadratic behavior and that means it's quite feasible to construct a case that showcase the quadratic behavior and behaves poorly. I was instead curious about more real-world or examples-in-practice you might run into, for example do you expect that you'd be working with multi-thousand opcode functions?

Nevertheless I'll elaborate some more on my concerns about the code organization here as well. I've left a review about various parts below. They're all tractable to fix in my opinion without too too much refactoring, but this is along the lines of what I was trying to avoid by having a third system of a sort to track pushes/pops/etc to keep in sync with everything else.

[alexcrichton]

I'm a little worried about putting this here due to the quadradic behavior of clone/==. For the same reasons your tests are showing it's quite slow, this would drastically slow down development/testing when this feature is enabled.

That being said it's also a really good test to have. One option perhaps might be something like a custom #[cfg(foo)] specified in CI but otherwise not tested anywhere. That way CI would test this, where it's presumably not a massive slowdown, but external consumers wouldn't test it.

[keithw]

Happy to do that if you want -- do you want a custom cfg or is a custom feature okay? (I may need help on best practices for the former.)

Full disclosure, at least on my laptop the performance penalty of running this on cargo test seems to be in the noise. (E.g. cargo test vs. cargo test -F wasmparser/try-op.) Probably because the spec testsuite doesn't have many large functions so the cost of cloning is negligible compared with everything else, especially given that this only runs on a debug build and with the feature enabled. But if you think it's better avoided, happy to make a custom feature or cfg for it.

[alexcrichton]

I'm thinking this'd be a good use case for a custom cfg given the performance penalty. I'm mostly concerned about other crates depending on wasmparser and inheriting this default-debug-mode behavior. For example Wasmtime ingesting a nontrivial wasm file in debug mode would take quadratic time here validating that file. I agree the cost is negligible in this repository which is where I think we could run this in CI here and still get the benefit of this strong assertion.

Specifically what this would look like:

• Invent a name for the cfg, e.g. debug_check_try_op
• Change this to #[cfg(all(debug_check_try_op, feature = "try-op"))]
• Extend the job matrix with a version that passes RUSTFLAGS: --cfg=debug_check_try_op in env (this'll also require changing this line to mix in the preexisting $RUSTFLAGS too)
• Expand the check-cfg list here to include cfg(debug_check_try_op) to allow-list this as a variable to check against.

[keithw]

Great, the roadmap was very helpful. :-) Done.

[keithw]

Hm well yes to a certain extent I understand that cloning has quadratic behavior and that means it's quite feasible to construct a case that showcase the quadratic behavior and behaves poorly. I was instead curious about more real-world or examples-in-practice you might run into, for example do you expect that you'd be working with multi-thousand opcode functions?

It's fair, but I guess the issue is that we're not building the tool for us -- it's an IDE for other people (especially beginners) to write code in, so I'm sort of thinking about the worst case text that some freshman could paste in that we'd want the tool to handle. :-/ I don't think we'll be encouraging people to write multi-thousand-opcode functions, but I also would like it to be hard for a user to paint themselves in a corner / drive over a performance cliff, hence how I got here...

Nevertheless I'll elaborate some more on my concerns about the code organization here as well. I've left a review about various parts below. They're all tractable to fix in my opinion without too too much refactoring, but this is along the lines of what I was trying to avoid by having a third system of a sort to track pushes/pops/etc to keep in sync with everything else.

Got it, and thank you. I appreciate the review and let me take a look at everything.

[alexcrichton]

Another possible idea to avoid transactions: instead of cloning at all in theory wasm validation is pretty fast so if you're willing to pay a 2x penalty then the implementation could validate once, keeping a count of how many operators were valid, and then upon seeing an invaild operator it could validate again, but only all the valid operators. That's sort of a "poor man's rewind" where it technically doesn't even need Clone (although you could use that and take snapshots every so often too).

Speed-wise validating twice in theory shouldn't be too bad (nowhere near quadratic) and it should in theory be a simple enough thing to maintain both externally (count ops + maybe validate twice) and internally (at most #[derive(Clone)] in a few places)

[keithw]

Another possible idea to avoid transactions: instead of cloning at all in theory wasm validation is pretty fast so if you're willing to pay a 2x penalty then the implementation could validate once, keeping a count of how many operators were valid, and then upon seeing an invaild operator it could validate again, but only all the valid operators.

No disagreement that this would be a lot simpler -- I definitely wanted to do something like this before ending up with the try_op approach, and I respect that this is adding complexity to support one unusual use case. I think the main reason this would be problematic for us is that we don't want to quit and restart after the first invalidity, because we're using this to compute (and visually display) the type of each operator. E.g. if the user writes:

(func
  i32.add
  f32.add
)

... we're using try_op to figure out that the i32.add operator has type i32 i32 → i32 and the f32.add operator has type f32 f32 → f32. And then it draws a little visual dataflow showing the flow of the operands and letting the user see why things are invalid. We do this by parsing the validator errors and prepending the necessary drop and x.const / ref.null operators until that operator validates (and we can see the types popped from and pushed to the operand stack), then moving on to the next operator. I think it would be unfortunate if we had to restart validation from the beginning of the function every time there is a missing/mismatched param.

[alexcrichton]

Ok that sounds reasonable enough yeah. Although If you're interested I think it would also be quite reasonable to provide more structured information from errors rather than forcing you to parse error strings. The error type in wasmparser is pretty under-developed but would be quite reasonable to add more variants/kinds to which are exposed through non-string types in Rust.

The current organization as-is I feel pretty good about as well, so with the custom-#[cfg] idea above I'll flag this for merge.

Thanks again for your work here, and thanks for being up for talking through your use case!