Add fixup function for nested pops in catch #4348
Conversation
This adds `EHUtils::handleBlockNestedPops`, which can be called at the end of passes that has a possibility to put `pop`s inside `block`s. This method assumes there exists a `pop` in a first-descendant line, even though it can be nested within a block. This allows a `pop` to be nested within a `block` or a `try`, but not a `loop`, since that means the `pop` can run multile times. In case of `if`, `pop` can exist only in its condition; if a `pop` is in its true or false body, that's not in the first-descendant line. To test this, this adds `passes/test_passes.cpp`, which is intended to contain multiple test passes that test a single (or more) utility functions separately. Without this kind of pass, it is hard to test various cases in which nested `pop`s can be generated in existing passes. This PR also adds `PassRegistry::registerTestPass`, which registers a pass that's intended only for internal testing and does not show up in `wasm-opt --help`. Fixes WebAssembly#4237.
src/ir/eh-utils.h
Outdated
| @@ -20,6 +20,8 @@ | |||
| namespace wasm { | |||
|
|
|||
| class Expression; | |||
| class Function; | |||
| class Module; | |||
There was a problem hiding this comment.
I don't think I noticed this before, but looks like it avoids including wasm.h by adding these forward declarations? Given there are 3 of them I think a single line that includes "wasm.h" would be shorter.
There was a problem hiding this comment.
Done. I tried to avoid includes if possible (just as general recommendations), but in this case that might not matter much anyway...
| try_->catchBodies[i] = | ||
| builder.makeSequence(builder.makeLocalSet(newLocal, pop), catchBody); | ||
| *popPtr = builder.makeLocalGet(newLocal, pop->type); | ||
| } |
There was a problem hiding this comment.
This should call handleNondefaultableLocals as well for the non-nullable local case, as I assume it's possible to pop a non-nullable type?
(All these corner cases stack up, sadly...)
There was a problem hiding this comment.
Oh right, thanks. I added $pop-non-defaultable-type-within-block to test this case too.
src/pass.h
Outdated
| std::unique_ptr<Pass> createPass(std::string name); | ||
| std::vector<std::string> getRegisteredNames(); | ||
| std::vector<std::string> getRegisteredNames(bool includeHidden = false); |
There was a problem hiding this comment.
How about an enum for Hidden/Visible? Can be used on line 58 too.
There was a problem hiding this comment.
I moved the hidden property to option, so this code is gone now.. PTAL at the new code and let me know if that looks fine.
|
Hmm, the |
| } | ||
| if (firstChild->is<Try>()) { | ||
| isPopNested = true; | ||
| } |
There was a problem hiding this comment.
How about switching the sequence of ifs to a chain of if-else, and then add WASM_UNREACHABLE at the end? Then if we add a new control flow instruction in the future it would error here, which could help avoid some future debugging.
| @@ -1,12 +1,18 @@ | |||
| ;; NOTE: Assertions have been generated by update_lit_checks.py and should not be edited. | |||
| ;; RUN: wasm-opt %s --no-validation --catch-pop-fixup -all -S -o - | filecheck %s | |||
| ;; RUN: wasm-opt %s --catch-pop-fixup --no-validation -all -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Maybe add a comment about why no-validation is used here?
This adds
EHUtils::handleBlockNestedPops, which can be called at theend of passes that has a possibility to put
pops insideblocks. Thismethod assumes there exists a
popin a first-descendant line, eventhough it can be nested within a block. This allows a
popto be nestedwithin a
blockor atry, but not aloop, since that means thepopcan run multile times. In case ofif,popcan exist only inits condition; if a
popis in its true or false body, that's not inthe first-descendant line.
This can be useful when optimization passes create blocks to do
transformations. Wrapping expressions wiith a block does not change
semantics most of the time, but if pops happen to be inside a block
generated by those passes, they can result in invalid binaries.
To test this, this adds
passes/test_passes.cpp, which is intended tocontain multiple test passes that test a single (or more) utility
functions separately. Without this kind of pass, it is hard to test
various cases in which nested
pops can be generated in existingpasses. This PR also adds
PassRegistry::registerTestPass, whichregisters a pass that's intended only for internal testing and does not
show up in
wasm-opt --help.Fixes #4237.