[WinEH] Apply funclet operand bundles to nounwind intrinsics that lower to function calls

[sgraenitz]

Summary

WinEHPrepare marks any function call from EH funclets as unreachable, if it's not a nounwind intrinsic and 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: https://github.com/gnustep/libobjc2/issues/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.

[sgraenitz]

Add missing objc_sync_exit to fix failing test Transforms/PreISelIntrinsicLowering/objc-arc.ll

[sgraenitz]

Running check-llvm and check-clang locally found no more failing tests. The issue is not limited to pre-ISel intrinsics (anymore) and I have to re-phrase a few comments. Apart from that, is there any more feedback? Thanks

[theraven]

LGTM, a couple of extra comments would help.

llvm/include/llvm/IR/IntrinsicInst.h

110

Minor nit, should be /// for a doc comment.

llvm/lib/IR/IntrinsicInst.cpp

61

I'm curious why memcpy and friends don't need to be on this list. Do they get expanded at a different point? Or is it that the front end inserts a memcpy call and the optimiser that turns them into intrinsics and back preserves operand bundles? It would be a good idea to have a comment explaining why these specific ones are on the list and not other libc (or math library) functions that may appear in cleanup blocks. From the code in InlineFunction.cpp, it looks as if this is a problem only for intrinsics that may throw, which excludes most of the C standard ones?

[sgraenitz]

Rebase and polish comments

[sgraenitz]

Hi @theraven thanks for reviewing!

llvm/lib/IR/IntrinsicInst.cpp

61

The name mayLowerToFunctionCall was proposed in a precursor to this review: https://reviews.llvm.org/D127962#inline-1227485. I agree that the term might appear overly general at first. I still followed the advice because:
(1) This controls assignment and propagation of operand bundles, which is a very general concept.
(2) From the perspective of IR transformations, only a few intrinsics actually lower to function calls (the majority remains intrinsics).
(3) The approach might be applied to other intrinsics, e.g. statepoints

However, this is not obvious and because other readers might have the same question, I extended my comment in the header like this:

Check if the intrinsic might lower into a regular function call in the course of IR transformations

Do you think that makes it more clear?

For additional context
Right now the list only contains ObjC ARC intrinsics. These are subject to PreISelIntrinsicLowering which means they are lowered to regular function calls before we run exception handling passes:
https://github.com/llvm/llvm-project/blob/c73adbad6a9964e0700865b7c786cc6885898c68/llvm/lib/CodeGen/TargetPassConfig.cpp#L1114-L1118

WinEHPrepare is an exception handling pass and it accepts call instructions in EH funclets only if they:

• target nounwind intrinsics or inline assembly
• have a matching funclet bundle operand

It declares other calls as "implausible instructions" and marks them unreachable, which caused the truncation bug that this patch aims to fix.

All ObjC ARC intrinsics have the nounwind attribute. My understanding is that the only user-code that might run in turn of an invocation can be destructors -- and they don't throw per definition.

The reason libc functions like memcpy are not in this list is that they are lowered at a later point in time. In the context of WinEHPrepare (and opt in general) they are always intrinsics.

[sgraenitz]

Pre-merge checks passed on Windows. Debian failures are unrelated. Locally on Ubuntu and macOS all tests pass. Getting ready to land this.

clang/lib/CodeGen/CGCall.cpp

llvm::CallInst *CodeGenFunction::EmitRuntimeCall(llvm::FunctionCallee callee,
                                                  const llvm::Twine &name) {
   return EmitRuntimeCall(callee, None, name);
 }
 
 // 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.
   if (!CurrentFuncletPad)
-    return BundleList;
+    return (SmallVector<llvm::OperandBundleDef, 1>());
 
-  // Skip intrinsics which cannot throw.
-  auto *CalleeFn = dyn_cast<llvm::Function>(Callee->stripPointerCasts());
-  if (CalleeFn && CalleeFn->isIntrinsic() && CalleeFn->doesNotThrow())
-    return BundleList;
+  // 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;
 }
 
 /// 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) {

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
+
+// 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]]) ]

llvm/include/llvm/IR/IntrinsicInst.h

bool isCommutative() const {
     case Intrinsic::fma:
     case Intrinsic::fmuladd:
       return true;
     default:
       return false;
     }
   }
 
-  // Checks if the intrinsic is an annotation.
+  /// Checks if the intrinsic is an annotation.
   bool isAssumeLikeIntrinsic() const {
     switch (getIntrinsicID()) {
     default: break;
     case Intrinsic::assume:
     case Intrinsic::sideeffect:
     case Intrinsic::pseudoprobe:
     case Intrinsic::dbg_declare:
     case Intrinsic::dbg_value:
     case Intrinsic::dbg_label:
     case Intrinsic::invariant_start:
     case Intrinsic::invariant_end:
     case Intrinsic::lifetime_start:
     case Intrinsic::lifetime_end:
     case Intrinsic::experimental_noalias_scope_decl:
     case Intrinsic::objectsize:
     case Intrinsic::ptr_annotation:
     case Intrinsic::var_annotation:
       return true;
     }
     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();
     return false;
   }
   static bool classof(const Value *V) {
     return isa<CallInst>(V) && classof(cast<CallInst>(V));
   }

llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp

 
 #include "llvm/CodeGen/PreISelIntrinsicLowering.h"
 #include "llvm/Analysis/ObjCARCInstKind.h"
 #include "llvm/Analysis/ObjCARCUtil.h"
 #include "llvm/CodeGen/Passes.h"
 #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"
 #include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 
 using namespace llvm;
 
if (objcarc::IsAlwaysTail(Kind))
     return CallInst::TCK_Tail;
   else if (objcarc::IsNeverTail(Kind))
     return CallInst::TCK_NoTail;
   return CallInst::TCK_None;
 }
 
 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;
 
   // If we haven't already looked up this function, check to see if the
   // program already contains a function with this name.
   Module *M = F.getParent();
   FunctionCallee FCache = M->getOrInsertFunction(NewFn, F.getFunctionType());
 
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/IR/IntrinsicInst.cpp

 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/IR/Statepoint.h"
 
 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.
 ///
 
 iterator_range<DbgVariableIntrinsic::location_op_iterator>
 DbgVariableIntrinsic::location_ops() const {
   auto *MD = getRawLocation();

llvm/lib/Transforms/Utils/InlineFunction.cpp

for (Instruction &I : BB) {
 
       if (NoAlias)
         I.setMetadata(LLVMContext::MD_noalias, MDNode::concatenate(
             I.getMetadata(LLVMContext::MD_noalias), NoAlias));
     }
   }
 }
 
+/// 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
 /// hold between the two version. It is necessary to create a deep clone of the
 /// metadata, putting the two versions in separate scope domains.
 class ScopedAliasMetadataDeepCloner {
   using MetadataMap = DenseMap<const MDNode *, TrackingMDNodeRef>;
   SetVector<const MDNode *> MD;
if (auto *II = dyn_cast<InvokeInst>(&CB)) {
     } else {
       HandleInlinedEHPad(II, &*FirstNewBlock, InlinedFunctionInfo);
     }
   }
 
   // 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
       // an EH pad that does.  Such an edge must be dynamically unreachable.
       // As such, we replace the cleanupret with unreachable.
       if (auto *CleanupRet = dyn_cast<CleanupReturnInst>(BB->getTerminator()))
         if (CleanupRet->unwindsToCaller() && EHPadForCallUnwindsLocally)
           changeToUnreachable(CleanupRet);

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

+; 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

llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll

+; 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