[llvm] da01a9d - [WebAssemblly] Fix EHPadStack update in fixCallUnwindMismatches

[Heejin Ahn via llvm-commits]

Author: Heejin Ahn
Date: 2021-02-17T12:14:11-08:00
New Revision: da01a9db8bb4d0ba362e1956ef7fa9bdd5e3b464

URL: https://github.com/llvm/llvm-project/commit/da01a9db8bb4d0ba362e1956ef7fa9bdd5e3b464
DIFF: https://github.com/llvm/llvm-project/commit/da01a9db8bb4d0ba362e1956ef7fa9bdd5e3b464.diff

LOG: [WebAssemblly] Fix EHPadStack update in fixCallUnwindMismatches

Updating `EHPadStack` with respect to `TRY` and `CATCH` instructions
have to be done after checking all other conditions, not before. Because
we did this before checking other conditions, when we encounter `TRY`
and we want to record the current mismatching range, we already have
popped up the entry from `EHPadStack`, which we need to access to record
the range.

The `baz` call in the added test needs try-delegate because the previous
TRY marker placement for `quux` was placed before `baz`, because `baz`'s
return value was stackified in RegStackify. If this wasn't stackified
this try-delegate is not strictly necessary, but at the moment it is not
easy to identify cases like this. I plan to transfer `nounwind`
attributes from the LLVM IR to prevent cases like this. The call in the
test does not have `unwind` attribute in order to test this bug, but in
many cases of this pattern the previous call has `nounwind` attribute.

Reviewed By: tlively

Differential Revision: https://reviews.llvm.org/D96711

Added: 
    

Modified: 
    llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
    llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
index 9f5290a90194..7035f741d74f 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp
@@ -1192,39 +1192,39 @@ bool WebAssemblyCFGStackify::fixCallUnwindMismatches(MachineFunction &MF) {
   for (auto &MBB : reverse(MF)) {
     bool SeenThrowableInstInBB = false;
     for (auto &MI : reverse(MBB)) {
-      if (MI.getOpcode() == WebAssembly::TRY)
-        EHPadStack.pop_back();
-      else if (WebAssembly::isCatch(MI.getOpcode()))
-        EHPadStack.push_back(MI.getParent());
       bool MayThrow = WebAssembly::mayThrow(MI);
 
       // If MBB has an EH pad successor and this is the last instruction that
       // may throw, this instruction unwinds to the EH pad and not to the
       // caller.
-      if (MBB.hasEHPadSuccessor() && MayThrow && !SeenThrowableInstInBB) {
+      if (MBB.hasEHPadSuccessor() && MayThrow && !SeenThrowableInstInBB)
         SeenThrowableInstInBB = true;
-        continue;
-      }
 
       // We wrap up the current range when we see a marker even if we haven't
       // finished a BB.
-      if (RangeEnd && WebAssembly::isMarker(MI.getOpcode())) {
+      else if (RangeEnd && WebAssembly::isMarker(MI.getOpcode()))
         RecordCallerMismatchRange(EHPadStack.back());
-        continue;
-      }
 
       // If EHPadStack is empty, that means it correctly unwinds to the caller
       // if it throws, so we're good. If MI does not throw, we're good too.
-      if (EHPadStack.empty() || !MayThrow)
-        continue;
+      else if (EHPadStack.empty() || !MayThrow) {
+      }
 
       // We found an instruction that unwinds to the caller but currently has an
       // incorrect unwind destination. Create a new range or increment the
       // currently existing range.
-      if (!RangeEnd)
-        RangeBegin = RangeEnd = &MI;
-      else
-        RangeBegin = &MI;
+      else {
+        if (!RangeEnd)
+          RangeBegin = RangeEnd = &MI;
+        else
+          RangeBegin = &MI;
+      }
+
+      // Update EHPadStack.
+      if (MI.getOpcode() == WebAssembly::TRY)
+        EHPadStack.pop_back();
+      else if (WebAssembly::isCatch(MI.getOpcode()))
+        EHPadStack.push_back(MI.getParent());
     }
 
     if (RangeEnd)

diff  --git a/llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll b/llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
index e527add55023..cd9c84691cd8 100644
--- a/llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
+++ b/llvm/test/CodeGen/WebAssembly/cfg-stackify-eh.ll
@@ -1099,8 +1099,35 @@ ehcleanup:                                        ; preds = %if.then
   cleanupret from %0 unwind to caller
 }
 
+; This crashed when updating EHPadStack within fixCallUniwindMismatch had a bug.
+; This should not crash and try-delegate has to be created around 'call @baz',
+; because the initial TRY placement for 'call @quux' was done before 'call @baz'
+; because 'call @baz''s return value is stackified.
+
+; CHECK-LABEL: test19
+; CHECK: try
+; CHECK:   try
+; CHECK:     call $[[RET:[0-9]+]]=, baz
+; CHECK:   delegate  1
+; CHECK:    call  quux, $[[RET]]
+; CHECK: catch_all
+; CHECK: end_try
+define void @test19() personality i8* bitcast (i32 (...)* @__gxx_wasm_personality_v0 to i8*) {
+entry:
+  %call = call i32 @baz()
+  invoke void @quux(i32 %call)
+          to label %invoke.cont unwind label %ehcleanup
+
+ehcleanup:                                        ; preds = %entry
+  %0 = cleanuppad within none []
+  cleanupret from %0 unwind to caller
+
+invoke.cont:                                      ; preds = %entry
+  unreachable
+}
+
 ; Check if the unwind destination mismatch stats are correct
-; NOSORT: 18 wasm-cfg-stackify    - Number of call unwind mismatches found
+; NOSORT: 19 wasm-cfg-stackify    - Number of call unwind mismatches found
 ; NOSORT:  3 wasm-cfg-stackify    - Number of catch unwind mismatches found
 
 declare void @foo()