Fixup block-nested pops even when EH is not enabled #7130
Conversation
While parsing a binary file, there may be pops that need to be fixed up even if EH is not (yet) enabled because the target features section has not been parsed yet. Previously `EHUtils::handleBlockNestedPops` did not do anything if EH was not enabled, so the binary parser would fail to fix up pops in that case. Fix the utility to run no matter what features are enabled and fix up its users so it is only called from optimization passes when EH is enabled. Fixes #7127.
| @@ -149,9 +149,6 @@ void handleBlockNestedPop(Try* try_, Function* func, Module& wasm) { | |||
| } | |||
|
|
|||
| void handleBlockNestedPops(Function* func, Module& wasm) { | |||
| if (!wasm.features.hasExceptionHandling()) { | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
I worry this is a significant regression. Before this PR, many passes would skip this pop handling in e.g. normal Emscripten output, just because the feature was not present. That is, this fixes parsing, but it is slowing down optimizations IIUC.
If the parsing issue is that we haven't seen the features section yet, perhaps we can fix that by doing what we now do with names - scan ahead for the features section first?
There was a problem hiding this comment.
We can do the check in those passes too, as the code here does, I guess - but if that's the best option, perhaps we can avoid the code duplication by adding handleBlockNestedPopsAfterParse (or a much better name hopefully) that does the feature check?
There was a problem hiding this comment.
What do you think about adding an extra optional parameter bool ignoreFeatures = false that would only be used by the parser code?
|
Done! |
| @@ -35,15 +35,15 @@ | |||
| ;; The fixup will hoist the 'pop' and create another local to store it right | |||
| ;; after 'catch'. | |||
|
|
|||
| ;; RUN: wasm-opt -all %s.wasm -S -o - | filecheck %s | |||
| ;; RUN: wasm-opt %s.wasm -S -o - | filecheck %s | |||
There was a problem hiding this comment.
How does this command not error in the validator, as the wasm uses EH?
There was a problem hiding this comment.
Oh, the features section? Which is the binary change I guess.
There was a problem hiding this comment.
I added a target features section enabling EH to the binary, so this test now reflects what happens in practice when Emscripten first parses a binary.
While parsing a binary file, there may be pops that need to be fixed up
even if EH is not (yet) enabled because the target features section has
not been parsed yet. Previously
EHUtils::handleBlockNestedPopsdid notdo anything if EH was not enabled, so the binary parser would fail to
fix up pops in that case. Add an optional parameter to override this
behavior so the parser can fix up pops unconditionally.
Fixes #7127.