[PATCH] Fix WinEHPrepare bug with multiple catch handlers

[Andy Kaylor]

Hi rnk, majnemer,

This fixes the bug with multiple catch handlers at the same level.  The test case looks like this:

void f() {
  try {
    may_throw();
  }
  catch (int) {
  }
  catch (float) {
  }
}

With this IR:

; Function Attrs: uwtable
define void @"\01?f@@YAXXZ"() #0 {
entry:
  %exn.slot = alloca i8*
  %ehselector.slot = alloca i32
  %0 = alloca float, align 4
  %1 = alloca i32, align 4
  invoke void @"\01?may_throw@@YAXXZ"()
          to label %invoke.cont unwind label %lpad

invoke.cont:                                      ; preds = %entry
  br label %try.cont

lpad:                                             ; preds = %entry
  %2 = landingpad { i8*, i32 } personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*)
          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.H
          catch %eh.HandlerMapEntry* @llvm.eh.handlermapentry.M
  %3 = extractvalue { i8*, i32 } %2, 0
  store i8* %3, i8** %exn.slot
  %4 = extractvalue { i8*, i32 } %2, 1
  store i32 %4, i32* %ehselector.slot
  br label %catch.dispatch

catch.dispatch:                                   ; preds = %lpad
  %sel = load i32, i32* %ehselector.slot
  %5 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.H to i8*)) #3
  %matches = icmp eq i32 %sel, %5
  br i1 %matches, label %catch2, label %catch.fallthrough

catch2:                                           ; preds = %catch.dispatch
  %exn3 = load i8*, i8** %exn.slot
  %6 = bitcast i32* %1 to i8*
  call void @llvm.eh.begincatch(i8* %exn3, i8* %6) #3
  call void @llvm.eh.endcatch() #3
  br label %try.cont

try.cont:                                         ; preds = %catch2, %catch, %invoke.cont
  ret void

catch.fallthrough:                                ; preds = %catch.dispatch
  %7 = call i32 @llvm.eh.typeid.for(i8* bitcast (%eh.HandlerMapEntry* @llvm.eh.handlermapentry.M to i8*)) #3
  %matches1 = icmp eq i32 %sel, %7
  br i1 %matches1, label %catch, label %eh.resume

catch:                                            ; preds = %catch.fallthrough
  %exn = load i8*, i8** %exn.slot
  %8 = bitcast float* %0 to i8*
  call void @llvm.eh.begincatch(i8* %exn, i8* %8) #3
  call void @llvm.eh.endcatch() #3
  br label %try.cont

eh.resume:                                        ; preds = %catch.fallthrough
  %exn4 = load i8*, i8** %exn.slot
  %sel5 = load i32, i32* %ehselector.slot
  %lpad.val = insertvalue { i8*, i32 } undef, i8* %exn4, 0
  %lpad.val6 = insertvalue { i8*, i32 } %lpad.val, i32 %sel5, 1
  resume { i8*, i32 } %lpad.val6
}

It's a small fix, but I thought it was worth pausing to talk about because the code involved seems potentially brittle.

The problem in this case was that the landing pad selector value was being loaded in one catch dispatch block but being referenced in two blocks.  Neither the landing pad block mapping code nor the catch handling code were expecting this.  For this case it was fairly simple to extend the code to recognize what was happening, but it raises a concern in my mind that the selector value, which we must be able to recognize for this pass to work properly, can be shuffled around through any manner of transformation.  I can't see why it would be manipulated, beyond the basic optimizations to avoid storing and reloading it, but it can happen.

Do you think this needs to be more robust?

REPOSITORY
  rL LLVM

http://reviews.llvm.org/D8682

Files:
  lib/CodeGen/WinEHPrepare.cpp

Index: lib/CodeGen/WinEHPrepare.cpp
===================================================================
--- lib/CodeGen/WinEHPrepare.cpp
+++ lib/CodeGen/WinEHPrepare.cpp
@@ -925,6 +925,26 @@
     return CloningDirector::CloneInstruction;
   }
 
+  if (auto *Compare = dyn_cast<CmpInst>(Inst)) {
+    // Look for compare instructions that use selector values that were not
+    // defined in the current block.  A series of related catch dispatch blocks
+    // will share a loaded selector value, but after the first dispatch block
+    // we will have started outlining after the value is loaded.  We can
+    // spot this case by looking at the compare operands.
+    for (auto &U : Compare->operands()) {
+      // Ignore any operands we've already mapped.
+      if (VMap.count(U.get()))
+        continue;
+      if (auto *Load = dyn_cast<LoadInst>(U.get())) {
+        if (LPadMap.mapIfSelectorLoad(Load))
+          VMap[Load] = ConstantInt::get(SelectorIDType, 1);
+        break;
+      }
+    }
+    // Whether we mapped a selector load above or not, the compare gets cloned.
+    return CloningDirector::CloneInstruction;
+  }
+
   // Nested landing pads will be cloned as stubs, with just the
   // landingpad instruction and an unreachable instruction. When
   // all landingpads have been outlined, we'll replace this with the
@@ -1461,12 +1481,19 @@
             continue;
           if (Inst == Compare || Inst == Branch)
             continue;
-          if (!Inst->hasOneUse() || (Inst->user_back() != Compare))
-            return createCleanupHandler(CleanupHandlerMap, BB);
+          // Loads of selector values may be used by multiple blocks, but if the
+          // loaded value is used in this block, it should be used by the
+          // compare instruction.
+          if (auto *Load = dyn_cast<LoadInst>(Inst)) {
+            for (auto *U : Load->users()) {
+              if (cast<Instruction>(U)->getParent() == BB && U != Compare)
+                return createCleanupHandler(CleanupHandlerMap, BB);
+            }
+            continue;
+          }
           if (match(Inst, m_Intrinsic<Intrinsic::eh_typeid_for>()))
             continue;
-          if (!isa<LoadInst>(Inst))
-            return createCleanupHandler(CleanupHandlerMap, BB);
+          return createCleanupHandler(CleanupHandlerMap, BB);
         }
         // The selector dispatch block should always terminate our search.
         assert(BB == EndBB);

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D8682.22838.patch
Type: text/x-patch
Size: 2507 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150328/4921c2a2/attachment.bin>