[WinEH] Ignore dynamic allocas of catch objects

[pengfei]

Summary

It seems D17823 assumes catch objects are always fixed objects.
Though dynamic alloca is rare, it exists in manually written IR, e.g., https://godbolt.org/z/a8xY91seG
We may not need to support it in WinEH, but we should ignore it to prevent later crash.

[rnk]

Sorry for the delay, I was OOO, and David Majnemer has moved on to other projects. In the future, consider adding @hans for more visibility for WinEH related changes to help find a reviewer.

I think maybe it would be best to handle this in WinEHPrepare, which is where we check if the third argument operand of the catchpad instruction is an alloca. We could check there that the alloca is static if it exists, and it might also be healthy to report a backend error if some non-null, non-alloca value exists. Currently we silently ignore the value if it is not an alloca. It would be consistent with our current behavior to silently ignore dynamic allocas there, if you don't want to go through the trouble of reporting an error.

The next best place to report an error would be the verifier, but the validity of the arguments to the catchpad depend on the personality, so that would require teaching the verifier about more WinEH semantics.

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp

173–174 ↗

(On Diff #496468)

This seems overly strict, I think it will assert if there is any dynamic alloca in a function using WinEH. I don't think that's what we want, we only want to assert if the catch object itself is a dynamic alloca, right?

[pengfei]

@rnk Glad to see you back and thanks for the suggestions! I'm busy in other work recently, will update when I have time. Thanks!

[pengfei]

In D143739#4332486, @rnk wrote:

Sorry for the delay, I was OOO, and David Majnemer has moved on to other projects. In the future, consider adding @hans for more visibility for WinEH related changes to help find a reviewer.

I think maybe it would be best to handle this in WinEHPrepare, which is where we check if the third argument operand of the catchpad instruction is an alloca. We could check there that the alloca is static if it exists, and it might also be healthy to report a backend error if some non-null, non-alloca value exists. Currently we silently ignore the value if it is not an alloca. It would be consistent with our current behavior to silently ignore dynamic allocas there, if you don't want to go through the trouble of reporting an error.

The next best place to report an error would be the verifier, but the validity of the arguments to the catchpad depend on the personality, so that would require teaching the verifier about more WinEH semantics.

Thanks for the guidances! Changed WinEHPrepare to ignore dynamic allocas.

[rnk]

lgtm

[Naville]

I'm not sure if I understand the fix issue correctly, given my very limited knowledge with WinEH. But wouldn't this result in a silent failure if DynAlloca is used?

[rnk]

In D143739#4363058, @Naville wrote:

I'm not sure if I understand the fix issue correctly, given my very limited knowledge with WinEH. But wouldn't this result in a silent failure if DynAlloca is used?

That's true, but it's consistent with passing other non-alloca values to the catchpad. The next best thing would be to provide a diagnostic in the verifier here, but it depends on knowledge of the personality function:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Verifier.cpp#L4261

[Naville]

Wouldn't that still results in unwind failure like 60914? Or what am I missing here?

EDIT:
What exactly does "going through the trouble or reporting an error" means? I do have time dedicated to this topic but I'm not entirely sure what to do here

[pengfei]

In D143739#4363241, @Naville wrote:

Wouldn't that still results in unwind failure like 60914? Or what am I missing here?

EDIT:
What exactly does "going through the trouble or reporting an error" means? I do have time dedicated to this topic but I'm not entirely sure what to do here

Guess D151209 is what you expected. It does catch a lot ill formated IR tests which would be crash with llc.

llvm/lib/CodeGen/WinEHPrepare.cpp

for (const CatchPadInst *CPI : Handlers) {
     WinEHHandlerType HT;
     Constant *TypeInfo = cast<Constant>(CPI->getArgOperand(0));
     if (TypeInfo->isNullValue())
       HT.TypeDescriptor = nullptr;
     else
       HT.TypeDescriptor = cast<GlobalVariable>(TypeInfo->stripPointerCasts());
     HT.Adjectives = cast<ConstantInt>(CPI->getArgOperand(1))->getZExtValue();
     HT.Handler = CPI->getParent();
-    if (auto *AI =
-            dyn_cast<AllocaInst>(CPI->getArgOperand(2)->stripPointerCasts()))
-      HT.CatchObj.Alloca = AI;
-    else
-      HT.CatchObj.Alloca = nullptr;
+    Value *V = CPI->getArgOperand(2)->stripPointerCasts();
+    auto *AI = dyn_cast<AllocaInst>(V);
+    assert((AI || isa<ConstantPointerNull>(V)) && "must be alloc or null ptr");
+    // Silently ignore dynamic allocas.
+    if (AI && AI->isStaticAlloca())
+      HT.CatchObj.Alloca = AI;
     TBME.HandlerArray.push_back(HT);
   }
   FuncInfo.TryBlockMap.push_back(TBME);
 }
 
 static BasicBlock *getCleanupRetUnwindDest(const CleanupPadInst *CleanupPad) {
   for (const User *U : CleanupPad->users())
     if (const auto *CRI = dyn_cast<CleanupReturnInst>(U))

llvm/test/CodeGen/X86/win-catchpad.ll

 ; X64-NEXT: .long   .Lfunc_begin1@IMGREL
 ; X64-NEXT: .long   -1
 ; X64-NEXT: .long   .Ltmp[[before_call]]@IMGREL+1
 ; X64-NEXT: .long   0
 ; X64-NEXT: .long   .Ltmp[[after_call]]@IMGREL+1
 ; X64-NEXT: .long   -1
 ; X64-NEXT: .long   "?catch$[[catchbb]]@?0?branch_to_normal_dest@4HA"@IMGREL
 ; X64-NEXT: .long   1
+
+%struct.widget = type { i8**, i8*, [8 x i8] }
+
+@global = external global %struct.widget
+
+;; Check no crash if catch object is a dynamic alloca.
+define dso_local void @baz() local_unnamed_addr #0 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+bb:
+  br i1 poison, label %bb1, label %bb7
+
+bb1:                                              ; preds = %bb
+  %tmp = alloca i32, align 4
+  invoke void @foo()
+          to label %bb6 unwind label %bb2
+
+bb2:                                              ; preds = %bb1
+  %tmp3 = catchswitch within none [label %bb4] unwind label %bb8
+
+bb4:                                              ; preds = %bb2
+  %tmp5 = catchpad within %tmp3 [%struct.widget* @global, i32 0, i32* %tmp]
+  unreachable
+
+bb6:                                              ; preds = %bb1
+  unreachable
+
+bb7:                                              ; preds = %bb
+  ret void
+
+bb8:                                              ; preds = %bb2
+  %tmp9 = cleanuppad within none []
+  unreachable
+}
+
+declare dso_local void @foo() local_unnamed_addr