[WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

[sgraenitz]

Summary

Unwinding ObjC code with automatic reference counting involves calls to intrinsics like llvm.obj.retain and llvm.objc.destroyWeak. Exception handling on Windows is based on funclets and WinEHPrepare only allows calls to nounwind intrinsics. This works just fine, except for ObjC nounwind intrinsics, because these are lowered into regular function calls in an earlier stage already (i.e. PreISelIntrinsicLoweringPass). Thus, WinEHPrepare accidentally drops them as implausible instructions and silently truncates certain funclets. Eventually, this causes crashes during unwinding on Windows with the GNUstep ObjC runtime: https://github.com/gnustep/libobjc2/issues/222

This patch forces the emission of a "funclet" bundle operand for calls to ObjC ARC intrinsics during codegen and lets PreISelIntrinsicLowering carry it over onto lowered replacement calls. This way all ObjC ARC calls survive WinEHPrepare through the FuncletBundleOperand check. The latter had to be adjusted in order to allow funclet pads to get optimized away.

[sgraenitz]

I guess testing must be split in two:

• Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC ARC runtime calls on Windows. Maybe that fits into clang/test/CodeGenObjC/gnu-exceptions.m?
• LLVM wants to check that WinEHPrepare handles these runtime calls correctly. For that maybe I can use llvm/test/CodeGen/X86/win-funclet-cfi.ll as a template and adjust it for the ObjC ARC case?

[rnk]

Thanks!

In D124762#3485478, @sgraenitz wrote:

I guess testing must be split in two:

• Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC ARC runtime calls on Windows. Maybe that fits into clang/test/CodeGenObjC/gnu-exceptions.m?

That seems like a good template for a new test. I think it probably deserves its own file. I imagine it will grow to be a more extensive test suite covering the interaction between GNUstep Objective C exceptions and WinEH.

• LLVM wants to check that WinEHPrepare handles these runtime calls correctly. For that maybe I can use llvm/test/CodeGen/X86/win-funclet-cfi.ll as a template and adjust it for the ObjC ARC case?

You could go that route, but I would probably start with simplified ObjC++ code, and iteratively modify that until you get the IR that has the funclets that you want to see. This is a decent starting template:

void maythrow();
void arcOpsInFunclet() {
  try {
    maythrow();
  }
  catch (...) {
    // Do things that generate ARC intrinsics, probably set up an `id` variable.
  }
}

Maybe you could make the test plain ObjC, but I don't know it very well.

Lastly, I think the inliner needs to be taught that these intrinsics need to carry operand bundles, otherwise this problem will arise after optimization. The inliner has the same logic here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2311

The test case for this should be:

struct Foo {
  inline __attribute__((always_inline)) ~Foo() { /* ARC ops to inline */ }
};
void maythrow();
void bar() {
  Foo o;
  maythrow();
}

llvm/lib/CodeGen/WinEHPrepare.cpp

966

Is this change necessary? It seems like it shouldn't be after the clang and preisel pass changes.

[sgraenitz]

Thanks for your help! I will work on testing in calendar week 21.

llvm/lib/CodeGen/WinEHPrepare.cpp

966

Yes, apparently it is necessary. There are cases, where CodeGenFunction::getBundlesForFunclet() registers a "funclet" bundle operand, but the FuncletPad here is null. My guess was it might be due to optimizations?

[sgraenitz]

Fix unchecked nullptr compiler crash and assertion

[sgraenitz]

LLVM CodeGen: check that presence of bundle operands avoids truncation

[sgraenitz]

Clang frontend: check that 'funclet' bundle operands are emitted for Pre-ISel intrinsics

[sgraenitz]

My follow-up got delayed, because I hit another bug, which appears to be a regression in release 14. This is why I wrote the tests for release/13.x and I still have to port them back to mainline, so this is *not yet ready to land*. As I don't expect it to cause big changes, however, I though it might be good to update the review anyway.

Rebased on release/13.x I am running the tests like this and both are passing:

> ninja llvm-config llvm-readobj llc FileCheck count not
> bin\llvm-lit.py --filter=win64-funclet-preisel-intrinsics test\CodeGen\X86
> ninja clang
> bin\llvm-lit.py --filter=arc-exceptions-seh tools\clang\test

@rnk What do you think about implementation, naming, etc. Is that ok?

In D124762#3486288, @rnk wrote:

Lastly, I think the inliner needs to be taught that these intrinsics need to carry operand bundles, otherwise this problem will arise after optimization. The inliner has the same logic here:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2311

Thanks for the note. Yes, I can reproduce that. I am not yet sure what's the best way to fix it though. Actually, I'd prefer to split it out into a separate review, so the two discussions can remain their individual focus. What do you think?

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

2

Note: There is another test without the -seh postfix that checks very similar situations for CodeGen with the macOS runtime.

llvm/lib/CodeGen/WinEHPrepare.cpp

966

Might that be due to the inliner issue then? I'd address it in the follow-up review if it's ok?

llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

54

Another side-note: Often times Clang just didn't emit anything here, so execution would run into whatever code comes next. The int3 I sometimes got might as well have been "accidental" padding.

[sgraenitz]

For illustration, truncations look like this (left: old CodeGen, right: with this fix applied)

[sgraenitz]

Update LLVM CodeGen test for mainline opaque pointers

[sgraenitz]

Fix accidental functional change that failed Clang test CodeGenCXX/microsoft-abi-eh-inlineasm.cpp

[sgraenitz]

Drop GNUstep assertion from getBundlesForFunclet(). Indeed a similar issue was observed before. See: D44640

[sgraenitz]

Close in favor of the D128190, which covers both cases at once: emit (was D124762) and inline (was D127962)

clang/lib/CodeGen/CGCall.cpp

 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/CodeGen/SwiftCallingConv.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Assumptions.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/CallingConv.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"

 }
 
 // Calls which may throw must have operand bundles indicating which funclet
 // they are nested within.
 SmallVector<llvm::OperandBundleDef, 1>
 CodeGenFunction::getBundlesForFunclet(llvm::Value *Callee) {
   SmallVector<llvm::OperandBundleDef, 1> BundleList;
   // There is no need for a funclet operand bundle if we aren't inside a
-  // funclet.
+  // funclet or the callee is not a function.
   if (!CurrentFuncletPad)
     return BundleList;
 
+  bool InsertFuncletOp = true;
+  if (auto *CalleeFn = dyn_cast<llvm::Function>(Callee->stripPointerCasts())) {
     // Skip intrinsics which cannot throw.
-  auto *CalleeFn = dyn_cast<llvm::Function>(Callee->stripPointerCasts());
-  if (CalleeFn && CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
-    return BundleList;
+    if (CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
+      InsertFuncletOp = false;
+
+    // Most ObjC ARC intrinics are lowered in PreISelIntrinsicLowering. Thus,
+    // WinEHPrepare will see them as regular calls. We need to set the funclet
+    // operand explicitly in this case to avoid accidental truncation of EH
+    // funclets on Windows.
+    if (CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow()) {
+      if (CGM.getTarget().getTriple().isOSWindows()) {
+        using namespace llvm::objcarc;
+        ARCInstKind CalleeKind = GetFunctionClass(CalleeFn);
+        if (!IsUser(CalleeKind) && CalleeKind != ARCInstKind::None)
+          InsertFuncletOp = true;
+      }
+    }
+  }
 
+  if (InsertFuncletOp)
     BundleList.emplace_back("funclet", CurrentFuncletPad);
+
   return BundleList;
 }
 
 /// Emits a simple call (never an invoke) to the given runtime function.
 llvm::CallInst *CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee callee,
                                                  ArrayRef<llvm::Value *> args,
                                                  const llvm::Twine &name) {
   llvm::CallInst *call = Builder.CreateCall(

clang/test/CodeGenObjCXX/arc-exceptions-seh.mm

+// RUN: %clang_cc1 -triple x86_64-pc-windows-msvc -emit-llvm -fobjc-arc -fexceptions -fobjc-exceptions -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s
+
+@class Ety;
+void opaque(void);
+void test_catch_preisel_intrinsic(void) {
+  @try {
+    opaque();
+  } @catch (Ety *ex) {
+    // Destroy ex when leaving catchpad. Emits calls to two intrinsic functions,
+    // that should both have a "funclet" bundle operand that refers to the
+    // catchpad's SSA value.
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_preisel_intrinsic
+//                ...
+// CHECK:       catch.dispatch:
+// CHECK-NEXT:    [[CATCHSWITCH:%[0-9]+]] = catchswitch within none
+//                ...
+// CHECK:       catch:
+// CHECK-NEXT:    [[CATCHPAD:%[0-9]+]] = catchpad within [[CATCHSWITCH]]
+// CHECK:         {{%[0-9]+}} = call {{.*}} @llvm.objc.retain{{.*}} [ "funclet"(token [[CATCHPAD]]) ]
+// CHECK:         call {{.*}} @llvm.objc.storeStrong{{.*}} [ "funclet"(token [[CATCHPAD]]) ]

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

if (CB->getCalledFunction() != &F) {
       continue;
     }
 
     auto *CI = cast<CallInst>(CB);
     assert(CI->getCalledFunction() && "Cannot lower an indirect call!");
 
     IRBuilder<> Builder(CI->getParent(), CI->getIterator());
     SmallVector<Value *, 8> Args(CI->args());
-    CallInst *NewCI = Builder.CreateCall(FCache, Args);
+    SmallVector<llvm::OperandBundleDef, 1> BundleList;
+    CI->getOperandBundlesAsDefs(BundleList);
+    CallInst *NewCI = Builder.CreateCall(FCache, Args, BundleList);
     NewCI->setName(CI->getName());
 
     // Try to set the most appropriate TailCallKind based on both the current
     // attributes and the ones that we could get from ObjCARC's special
     // knowledge of the runtime functions.
     //
     // std::max respects both requirements of notail and tail here:
     // * notail on either the call or from ObjCARC becomes notail

llvm/lib/CodeGen/WinEHPrepare.cpp

for (BasicBlock *BB : BlocksInFunclet) {
         auto *CB = dyn_cast<CallBase>(&I);
         if (!CB)
           continue;
 
         Value *FuncletBundleOperand = nullptr;
         if (auto BU = CB->getOperandBundle(LLVMContext::OB_funclet))
           FuncletBundleOperand = BU->Inputs.front();
 
-        if (FuncletBundleOperand == FuncletPad)
+        if (!FuncletPad || FuncletBundleOperand == FuncletPad)
           continue;
 
         // Skip call sites which are nounwind intrinsics or inline asm.
         auto *CalledFn =
             dyn_cast<Function>(CB->getCalledOperand()->stripPointerCasts());
         if (CalledFn && ((CalledFn->isIntrinsic() && CB->doesNotThrow()) ||
                          CB->isInlineAsm()))
           continue;

llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll

+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; Reduced IR generated from ObjC++ source:
+;
+;     @class Ety;
+;     void opaque(void);
+;     void test_catch(void) {
+;       @try {
+;         opaque();
+;       } @catch (Ety *ex) {
+;         // Destroy ex when leaving catchpad. This emits calls to two intrinsic
+;         // functions, llvm.objc.retain and llvm.objc.storeStrong, but only one
+;         // is required to trigger the funclet truncation.
+;       }
+;     }
+
+define void @test_catch() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex2 = alloca ptr, align 8
+  invoke void @opaque() to label %invoke.cont unwind label %catch.dispatch
+
+catch.dispatch:
+  %0 = catchswitch within none [label %catch] unwind to caller
+
+invoke.cont:
+  unreachable
+
+catch:
+  %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+  call void @llvm.objc.storeStrong(ptr %ex2, ptr null) [ "funclet"(token %1) ]
+  catchret from %1 to label %catchret.dest
+
+catchret.dest:
+  ret void
+}
+
+declare void @opaque()
+declare void @llvm.objc.storeStrong(ptr, ptr) #0
+declare i32 @__CxxFrameHandler3(...)
+
+attributes #0 = { nounwind }
+
+; llvm.objc.storeStrong is a Pre-ISel intrinsic, which used to cause truncations
+; when it occurred in SEH funclets like catchpads:
+;     CHECK: # %catch
+;     CHECK: pushq   %rbp
+;     CHECK: .seh_pushreg %rbp
+;            ...
+;     CHECK: .seh_endprologue
+;
+; At this point the code used to be truncated (and sometimes terminated with an
+; int3 opcode):
+;     CHECK-NOT: int3
+;
+; Instead, the call to objc_storeStrong should be emitted:
+;     CHECK: leaq -24(%rbp), %rcx
+;     CHECK: xorl %edx, %edx
+;     CHECK: callq  objc_storeStrong
+;        ...
+; 
+; This is the end of the funclet:
+;     CHECK: popq %rbp
+;     CHECK: retq                                    # CATCHRET
+;            ...
+;     CHECK: .seh_handlerdata
+;            ...
+;     CHECK: .seh_endproc