[WinEH] Apply funclet operand bundles to nounwind intrinsics that low…

…er to function calls in the course of IR transforms WinEHPrepare marks any function call from EH funclets as unreachable, if it's not a nounwind intrinsic or has no proper funclet bundle operand. This affects ARC intrinsics on Windows, because they are lowered to regular function calls in the PreISelIntrinsicLowering pass. It caused silent binary truncations and crashes during unwinding with the GNUstep ObjC runtime: gnustep/libobjc2#222 This patch adds a new function `llvm::IntrinsicInst::mayLowerToFunctionCall()` that aims to collect all affected intrinsic IDs. * Clang CodeGen uses it to determine whether or not it must emit a funclet bundle operand. * PreISelIntrinsicLowering asserts that the function returns true for all ObjC runtime calls it lowers. * LLVM uses it to determine whether or not a funclet bundle operand must be propagated to inlined call sites. Reviewed By: theraven Differential Revision: https://reviews.llvm.org/D128190
draperlaboratory · Oct 5, 2022 · 463592e · 463592e
1 parent 22ff434
commit 463592e
Show file tree

Hide file tree

Showing 8 changed files with 245 additions and 38 deletions.
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
@@ -4473,17 +4473,22 @@ llvm::CallInst *CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee callee,
 // 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.
   if (!CurrentFuncletPad)
-    return BundleList;
-
-  // Skip intrinsics which cannot throw.
-  auto *CalleeFn = dyn_cast<llvm::Function>(Callee->stripPointerCasts());
-  if (CalleeFn && CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
-    return BundleList;
+    return (SmallVector<llvm::OperandBundleDef, 1>());
+
+  // Skip intrinsics which cannot throw (as long as they don't lower into
+  // regular function calls in the course of IR transformations).
+  if (auto *CalleeFn = dyn_cast<llvm::Function>(Callee->stripPointerCasts())) {
+    if (CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow()) {
+      auto IID = CalleeFn->getIntrinsicID();
+      if (!llvm::IntrinsicInst::mayLowerToFunctionCall(IID))
+        return (SmallVector<llvm::OperandBundleDef, 1>());
+    }
+  }
 
+  SmallVector<llvm::OperandBundleDef, 1> BundleList;
   BundleList.emplace_back("funclet", CurrentFuncletPad);
   return BundleList;
 }

diff --git a/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm b/clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,29 @@
+// 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
+
+// WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+// regular function calls in the course of IR transformations.
+//
+// This is the case for ObjC ARC runtime intrinsics. Test that clang emits the
+// funclet tokens for llvm.objc.retain and llvm.objc.storeStrong and that they
+// refer to their catchpad's SSA value.
+
+@class Ety;
+void opaque(void);
+void test_catch_with_objc_intrinsic(void) {
+  @try {
+    opaque();
+  } @catch (Ety *ex) {
+    // Destroy ex when leaving catchpad. This emits calls to intrinsic functions
+    // llvm.objc.retain and llvm.objc.storeStrong
+  }
+}
+
+// CHECK-LABEL: define{{.*}} void {{.*}}test_catch_with_objc_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]]) ]
diff --git a/llvm/include/llvm/IR/IntrinsicInst.h b/llvm/include/llvm/IR/IntrinsicInst.h
@@ -84,7 +84,7 @@ class IntrinsicInst : public CallInst {
     }
   }
 
-  // Checks if the intrinsic is an annotation.
+  /// Checks if the intrinsic is an annotation.
   bool isAssumeLikeIntrinsic() const {
     switch (getIntrinsicID()) {
     default: break;
@@ -107,7 +107,11 @@ class IntrinsicInst : public CallInst {
     return false;
   }
 
-  // Methods for support type inquiry through isa, cast, and dyn_cast:
+  /// Check if the intrinsic might lower into a regular function call in the
+  /// course of IR transformations
+  static bool mayLowerToFunctionCall(Intrinsic::ID IID);
+
+  /// Methods for support type inquiry through isa, cast, and dyn_cast:
   static bool classof(const CallInst *I) {
     if (const Function *CF = I->getCalledFunction())
       return CF->isIntrinsic();

diff --git a/llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp b/llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
@@ -18,6 +18,7 @@
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/InitializePasses.h"
@@ -69,6 +70,8 @@ static CallInst::TailCallKind getOverridingTailCallKind(const Function &F) {
 
 static bool lowerObjCCall(Function &F, const char *NewFn,
                           bool setNonLazyBind = false) {
+  assert(IntrinsicInst::mayLowerToFunctionCall(F.getIntrinsicID()) &&
+         "Pre-ISel intrinsics do lower into regular function calls");
   if (F.use_empty())
     return false;
 
@@ -107,7 +110,9 @@ static bool lowerObjCCall(Function &F, const char *NewFn,
 
     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

diff --git a/llvm/lib/IR/IntrinsicInst.cpp b/llvm/lib/IR/IntrinsicInst.cpp
@@ -32,6 +32,39 @@
 
 using namespace llvm;
 
+bool IntrinsicInst::mayLowerToFunctionCall(Intrinsic::ID IID) {
+  switch (IID) {
+  case Intrinsic::objc_autorelease:
+  case Intrinsic::objc_autoreleasePoolPop:
+  case Intrinsic::objc_autoreleasePoolPush:
+  case Intrinsic::objc_autoreleaseReturnValue:
+  case Intrinsic::objc_copyWeak:
+  case Intrinsic::objc_destroyWeak:
+  case Intrinsic::objc_initWeak:
+  case Intrinsic::objc_loadWeak:
+  case Intrinsic::objc_loadWeakRetained:
+  case Intrinsic::objc_moveWeak:
+  case Intrinsic::objc_release:
+  case Intrinsic::objc_retain:
+  case Intrinsic::objc_retainAutorelease:
+  case Intrinsic::objc_retainAutoreleaseReturnValue:
+  case Intrinsic::objc_retainAutoreleasedReturnValue:
+  case Intrinsic::objc_retainBlock:
+  case Intrinsic::objc_storeStrong:
+  case Intrinsic::objc_storeWeak:
+  case Intrinsic::objc_unsafeClaimAutoreleasedReturnValue:
+  case Intrinsic::objc_retainedObject:
+  case Intrinsic::objc_unretainedObject:
+  case Intrinsic::objc_unretainedPointer:
+  case Intrinsic::objc_retain_autorelease:
+  case Intrinsic::objc_sync_enter:
+  case Intrinsic::objc_sync_exit:
+    return true;
+  default:
+    return false;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 /// DbgVariableIntrinsic - This is the common base class for debug info
 /// intrinsics for variables.

diff --git a/llvm/lib/Transforms/Utils/InlineFunction.cpp b/llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -825,6 +825,35 @@ static void PropagateCallSiteMetadata(CallBase &CB, Function::iterator FStart,
   }
 }
 
+/// Bundle operands of the inlined function must be added to inlined call sites.
+static void PropagateOperandBundles(Function::iterator InlinedBB,
+                                    Instruction *CallSiteEHPad) {
+  for (Instruction &II : llvm::make_early_inc_range(*InlinedBB)) {
+    CallBase *I = dyn_cast<CallBase>(&II);
+    if (!I)
+      continue;
+    // Skip call sites which already have a "funclet" bundle.
+    if (I->getOperandBundle(LLVMContext::OB_funclet))
+      continue;
+    // Skip call sites which are nounwind intrinsics (as long as they don't
+    // lower into regular function calls in the course of IR transformations).
+    auto *CalledFn =
+        dyn_cast<Function>(I->getCalledOperand()->stripPointerCasts());
+    if (CalledFn && CalledFn->isIntrinsic() && I->doesNotThrow() &&
+        !IntrinsicInst::mayLowerToFunctionCall(CalledFn->getIntrinsicID()))
+      continue;
+
+    SmallVector<OperandBundleDef, 1> OpBundles;
+    I->getOperandBundlesAsDefs(OpBundles);
+    OpBundles.emplace_back("funclet", CallSiteEHPad);
+
+    Instruction *NewInst = CallBase::Create(I, OpBundles, I);
+    NewInst->takeName(I);
+    I->replaceAllUsesWith(NewInst);
+    I->eraseFromParent();
+  }
+}
+
 namespace {
 /// Utility for cloning !noalias and !alias.scope metadata. When a code region
 /// using scoped alias metadata is inlined, the aliasing relationships may not
@@ -2304,38 +2333,12 @@ llvm::InlineResult llvm::InlineFunction(CallBase &CB, InlineFunctionInfo &IFI,
   // Update the lexical scopes of the new funclets and callsites.
   // Anything that had 'none' as its parent is now nested inside the callsite's
   // EHPad.
-
   if (CallSiteEHPad) {
     for (Function::iterator BB = FirstNewBlock->getIterator(),
                             E = Caller->end();
          BB != E; ++BB) {
-      // Add bundle operands to any top-level call sites.
-      SmallVector<OperandBundleDef, 1> OpBundles;
-      for (Instruction &II : llvm::make_early_inc_range(*BB)) {
-        CallBase *I = dyn_cast<CallBase>(&II);
-        if (!I)
-          continue;
-
-        // Skip call sites which are nounwind intrinsics.
-        auto *CalledFn =
-            dyn_cast<Function>(I->getCalledOperand()->stripPointerCasts());
-        if (CalledFn && CalledFn->isIntrinsic() && I->doesNotThrow())
-          continue;
-
-        // Skip call sites which already have a "funclet" bundle.
-        if (I->getOperandBundle(LLVMContext::OB_funclet))
-          continue;
-
-        I->getOperandBundlesAsDefs(OpBundles);
-        OpBundles.emplace_back("funclet", CallSiteEHPad);
-
-        Instruction *NewInst = CallBase::Create(I, OpBundles, I);
-        NewInst->takeName(I);
-        I->replaceAllUsesWith(NewInst);
-        I->eraseFromParent();
-
-        OpBundles.clear();
-      }
+      // Add bundle operands to inlined call sites.
+      PropagateOperandBundles(BB, CallSiteEHPad);
 
       // It is problematic if the inlinee has a cleanupret which unwinds to
       // caller and we inline it into a call site which doesn't unwind but into

diff --git a/llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll b/llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,77 @@
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the code generator will emit the function call and not consider it
+; an "implausible instruciton". In the past this silently truncated code on
+; exception paths and caused crashes at runtime.
+;
+; Reduced IR generated from ObjC++ source:
+;
+;     @class Ety;
+;     void opaque(void);
+;     void test_catch_with_objc_intrinsic(void) {
+;       @try {
+;         opaque();
+;       } @catch (Ety *ex) {
+;         // Destroy ex when leaving catchpad. This would emit calls to two
+;         // intrinsic functions: llvm.objc.retain and llvm.objc.storeStrong
+;       }
+;     }
+;
+; llvm.objc.retain and llvm.objc.storeStrong both lower into regular function
+; calls before ISel. We only need one of them to trigger funclet truncation
+; during codegen:
+
+define void @test_catch_with_objc_intrinsic() 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 }
+
+; EH catchpad with SEH prologue:
+;     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
diff --git a/llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll b/llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,51 @@
+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH requires funclet tokens on nounwind intrinsics if they can lower to
+; regular function calls in the course of IR transformations.
+;
+; Test that the inliner propagates funclet tokens to such intrinsics when
+; inlining into EH funclets, i.e.: llvm.objc.storeStrong inherits the "funclet"
+; token from inlined_fn.
+
+define void @inlined_fn(ptr %ex) #1 {
+entry:
+  call void @llvm.objc.storeStrong(ptr %ex, ptr null)
+  ret void
+}
+
+define void @test_catch_with_inline() personality ptr @__CxxFrameHandler3 {
+entry:
+  %exn.slot = alloca ptr, align 8
+  %ex = 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 @inlined_fn(ptr %ex) [ "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 }
+attributes #1 = { alwaysinline }
+
+; After inlining, llvm.objc.storeStrong inherited the "funclet" token:
+;
+;   CHECK-LABEL:  define void @test_catch_with_inline()
+;                   ...
+;   CHECK:        catch:
+;   CHECK-NEXT:     %1 = catchpad within %0 [ptr null, i32 64, ptr %exn.slot]
+;   CHECK-NEXT:     call void @llvm.objc.storeStrong(ptr %ex, ptr null) [ "funclet"(token %1) ]
+;   CHECK-NEXT:     catchret from %1 to label %catchret.dest