Basic EH instrucion support for the new spec #3487
Conversation
This updates `try`-`catch`-`catch_all` and `rethrow` instructions to match the new spec. `delegate` is not included. Now `Try` contains not a single `catchBody` expression but a vector of catch bodies and events. This updates most existing routines, optimizations, and tests modulo the interpreter and the CFG traversal. Because the interpreter has not been updated yet, the EH spec test is temporarily disabled in check.py. Also, because the CFG traversal for EH is not yet updated, several EH tests in `rse_all-features.wast`, which uses CFG traversal, are temporarily commented out.
|
Currently binaryen.js tests fail. They will pass after #3486 lands. |
src/wasm/wasm-validator.cpp
Outdated
| // TODO Validate depth field if it becomes necessary in future. This requires | ||
| // a separate stack for 'catch', and the current LLVM toolchain only generates | ||
| // depth 0 for C++ support. |
There was a problem hiding this comment.
Would it be worth validating that depth is 0 for now? Or do you plan to resolve this TODO in the near future?
There was a problem hiding this comment.
Validating this requires maintaining a separate stack within the validator, which is not difficult but I'm not very sure if it's worth it at the moment, so I don't have a plan to add that in near future, but if you think it's important we can change priorities. But at the same time I think rejecting values other than 0 can be risky, because binaryen is not guaranteed to take wasm code produced only by our LLVM toolchain.
There was a problem hiding this comment.
My thinking here is that it would be better to reject a valid program that uses non-zero depths than to accidentally and silently mis-optimize the program because we don't reason about non-zero depths right now. Or do we handle non-zero depths correctly in the optimization passes already?
There was a problem hiding this comment.
I don't think optimization passes will change the depth field, because they don't have any way of reasoning about that field. But yeah, there can be optimizations that remove or alter try-catch structure that are targeted by rethrow. I can't think of any optimization pass that does that right now though.
For one, Vacuum removes try-catch altogether if the body of try does not throw. But this case is OK for rethrow because it removes the whole catch body. But there can be other passes that might interfere with rethrow, as you pointed out.
I'll reject non-zero depths for now for safe measure.
Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>
Oh, I would appreciate that 😀 |
This updates
try-catch-catch_allandrethrowinstructions tomatch the new spec.
delegateis not included. NowTrycontains not asingle
catchBodyexpression but a vector of catchbodies and events.
This updates most existing routines, optimizations, and tests modulo the
interpreter and the CFG traversal. Because the interpreter has not been
updated yet, the EH spec test is temporarily disabled in check.py. Also,
because the CFG traversal for EH is not yet updated, several EH tests in
rse_all-features.wast, which uses CFG traversal, are temporarilycommented out.
Also added a few more tests in existing EH test functions in
test/passes. In the previous spec,
catchwas catching all exceptionsso it was assumed that anything
trybody throws is caught by itscatch, but now we can assume the same only if there is acatch_all.Newly added tests test cases when there is a
catch_alland cases thereare only
catches separately.