[WinEHPrepare] Propagate funclet tokens when inlining into EH funclets

[sgraenitz]

Summary

WinEH requires funclet tokens for (nounwind) ObjC++ ARC intrinsics, because they are subject to pre-ISel lowering. That's because they appear as regular function calls for subsequent passes (like WinEHPrepare). Without funclet token they would be consider them implausible instrucitons and marked as unreachable. Affected EH funclets would get truncated silently, which causes unpredictable crashes at runtime.

Thus, when we target WinEH and generate calls to pre-ISel intrinsics from EH funclets, we emit funclet tokens explicitly. My previous patch D124762 implements that.

Now, the inliner has to propagate funclet tokens to such intrinsics, if they get inlined into EH funclets. That's what this patch implements.

[sgraenitz]

Limit explicit propagation to actual pre-ISel intrinsics

[sgraenitz]

This is the best solution I found so far. The funclet bundle operand (aka. token) of the inlinee function will be propagated to the set of intrinsics that is (as of today) subject to pre-ISel lowering. This only applies when targeting WinEH and only for functions inlined into EH funclets.

However, there is no check that makes sure the actual set of intrinsics is in sync with the condition I am using here. See my inline comment for a proposal.

llvm/lib/Transforms/Utils/InlineFunction.cpp

2313–2324

We could simplify the condition here by adding a new helper function in ObjCARCInstKind.h/.cpp:

bool llvm::objcarc::IsPreISelIntrinsic(Function *F) {
  ARCInstKind K = GetFunctionClass(F);
  return !IsUser(K) && K != ARCInstKind::None;
}

It could be used in D124762 CGCall.cpp as well as in PreISelIntrinsicLowering.cpp to assert the set of pre-ISel intrinsics is in sync with what we do here. This would also connect the three pieces of code explicitly, so it's easier for others to see their relation.

[rnk]

Thanks, sorry for the delay.

llvm/lib/Transforms/Utils/InlineFunction.cpp

2313–2324

I think the generic predicate we are looking should be called something like IntrinsicInst::mayLowerToFunctionCall, and it would be true for these ObjC ARC intrinsics, statepoint, and anything else like that. Basically, we should apply funclet operand bundles to intrinsics that lower to function calls.

2317

The personality check should not be necessary. We check CallSiteEHPad above, and that's enough to imply that the current function uses funclet bundle operands. (Wasm, SEH, C++ EH)

[sgraenitz]

Thanks for your feedback.

In D127962#3590140, @rnk wrote:

I think the generic predicate we are looking should be called something like IntrinsicInst::mayLowerToFunctionCall, and it would be true for these ObjC ARC intrinsics, statepoint, and anything else like that. Basically, we should apply funclet operand bundles to intrinsics that lower to function calls.

I was not aware it would be such a broad issue! So far, I was trying to narrow the condition to my ObjC ARC use-case as close as possible. D128190 aims to apply bundle operands correctly for intrinsics that lower to function calls and covers both cases, emit and inline, at once. If this is ok, I can close the two reviews here.

llvm/lib/Transforms/Utils/InlineFunction.cpp

2317

Alright, I assume then it's not necessary in clang/lib/CodeGen/CGCall.cpp either.

[sgraenitz]

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

llvm/lib/Transforms/Utils/InlineFunction.cpp

if (CalledPersonality) {
     //       supersets of others and can be used in place of the other.
     else if (CalledPersonality != CallerPersonality)
       return InlineResult::failure("incompatible personality");
   }
 
   // We need to figure out which funclet the callsite was in so that we may
   // properly nest the callee.
   Instruction *CallSiteEHPad = nullptr;
+  EHPersonality Personality = EHPersonality::Unknown;
   if (CallerPersonality) {
-    EHPersonality Personality = classifyEHPersonality(CallerPersonality);
+    Personality = classifyEHPersonality(CallerPersonality);
     if (isScopedEHPersonality(Personality)) {
       Optional<OperandBundleUse> ParentFunclet =
           CB.getOperandBundle(LLVMContext::OB_funclet);
       if (ParentFunclet)
         CallSiteEHPad = cast<FuncletPadInst>(ParentFunclet->Inputs.front());
 
       // OK, the inlining site is legal.  What about the target function?
 
for (Function::iterator BB = FirstNewBlock->getIterator(),
          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.
+        // Skip call sites which are nounwind intrinsics
         auto *CalledFn =
             dyn_cast<Function>(I->getCalledOperand()->stripPointerCasts());
-        if (CalledFn && CalledFn->isIntrinsic() && I->doesNotThrow())
+        if (CalledFn && CalledFn->isIntrinsic() && I->doesNotThrow()) {
+          // When targeting WinEH, we must propagate funclet tokens to pre-ISel
+          // intrinsics, if they get inlined into EH funclets. Otherwise,
+          // WinEHPrepare would mark them unreachable and cause truncations.
+          bool PropagateExplicitly = false;
+          if (Personality == EHPersonality::MSVC_CXX) {
+            using namespace llvm::objcarc;
+            ARCInstKind Kind = GetFunctionClass(CalledFn);
+            if (!IsUser(Kind) && Kind != ARCInstKind::None)
+              PropagateExplicitly = true;
+          }
+          if (!PropagateExplicitly)
             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);
 

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

+; RUN: opt -S -always-inline -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+
+; WinEH doesn't require funclet tokens on nounwind intrinsics per se. ObjC++ ARC
+; intrinsics are a special case, because they are subject to pre-ISel lowering.
+; They appear as regular function calls for subsequent passes, like WinEHPrepare
+; which would consider them implausible instrucitons and mark them unreachable.
+; Affected EH funclets would get truncated silently, which causes unpredictable
+; crashes at runtime.
+;
+; Thus, when we target WinEH and generate calls to pre-ISel intrinsics from EH
+; funclets, we emit funclet tokens explicitly.
+;
+; The inliner has to propagate funclet tokens to such intrinsics, if they get
+; inlined into EH funclets.
+
+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 }
+
+; 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