[PATCH] D48345: [WebAssembly] Fix unwind destination mismatches in CFG stackify
Derek Schuff via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 19 15:22:30 PDT 2019
dschuff added inline comments.
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:75
+ // Newly appended BB at the end of the function for special purpose
+ MachineBasicBlock *AppendixBB = nullptr;
----------------
This comment is pretty vague. I guess this is one block for each function, and is now shared for creating a correct signature for fallthrough returns, and to use as a target for rethrows that need to unwind to the caller, but are trapped inside an outer try/catch?
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:694
+
+ // Linearing the control flow by placing TRY / END_TRY markers can create
+ // mismatches in unwind destinations. There are two kinds of mismatches we
----------------
s/Linearing/Linearizing
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:730
+ // right after 'end_try', which means we extract the handler body out of
+ // the catch block. We do this because this handler boy should be somewhere
+ // branch-eable from the inner scope.
----------------
s/boy/body
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:764
+ // value created by catches. That's because we don't support yielding values
+ // from a block. in LLVM machine IR, even though it is supported by wasm.
+ // Delete unnecessary local.get/local.sets once yielding values from a block
----------------
remove the period after "block"
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:834
+ // there can be multiple cases:
+ // 1. We have -wasm-disable-explicit-locals set.
+ // 2. We don't have -wasm-disable-explicit-locals set.
----------------
It seems unfortunate that we have to care about whether explicit locals is turned on. Is that just because this pass runs after `createWebAssemblyExplicitLocals`? Does the order matter? e.g. would it be simpler to run it after CfgStackify? Or, could we just run it again if this pass changes anything?
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:869
+ !WebAssembly::mayThrow(MI))
+ continue;
+ SeenThrowableInstInBB = true;
----------------
`continue` here will just skip to the next instruction, but the comment says it will skip the rest of the BB (which makes sense), should this be `break` instead? Or do we need to keep going to find the try/catch?
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:943
+ assert(EHPadStack.empty());
+ // We don't any unwind destination mismatches to resolve.
+ if (UnwindDestToTryRanges.empty())
----------------
s/don't/don't have
================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1012
+ }
+ if (MI.getOpcode() == WebAssembly::LOCAL_SET_EXCEPT_REF) {
+ assert(Catch && MI.getOperand(1).getReg() == Info.Reg);
----------------
these could be `else if`?
================
Comment at: test/CodeGen/WebAssembly/cfg-stackify-eh-mismatch.ll:41
+
+define void @test0() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+bb0:
----------------
Any weird corner cases it would be worthwhile to test for? Mixing both kinds of mismatches? A case with uwind-to-caller and block signature needed? Would multiple nesting of mismatches affect what happens?
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D48345/new/
https://reviews.llvm.org/D48345
More information about the llvm-commits
mailing list