sgraenitz created this revision.
sgraenitz added reviewers: rnk, theraven, DHowett-MSFT.
Herald added subscribers: jsji, pengfei, hiraditya.
Herald added a project: All.
sgraenitz requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D128190
Files:
clang/lib/CodeGen/CGCall.cpp
clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
llvm/include/llvm/IR/IntrinsicInst.h
llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
llvm/lib/IR/IntrinsicInst.cpp
llvm/lib/Transforms/Utils/InlineFunction.cpp
llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
Index: llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
===================================================================
--- /dev/null
+++ llvm/test/Feature/OperandBundles/inliner-funclet-wineh.ll
@@ -0,0 +1,55 @@
+; 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
Index: llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/X86/win64-funclet-preisel-intrinsics.ll
@@ -0,0 +1,68 @@
+; 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
Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===================================================================
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -825,6 +825,35 @@
}
}
+/// Add bundle operands of the inlinded function 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 and don't lower into
+ // regular function calls.
+ 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
@@ -2293,38 +2322,12 @@
// 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
Index: llvm/lib/IR/IntrinsicInst.cpp
===================================================================
--- llvm/lib/IR/IntrinsicInst.cpp
+++ llvm/lib/IR/IntrinsicInst.cpp
@@ -32,6 +32,38 @@
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:
+ return true;
+ default:
+ return false;
+ }
+}
+
//===----------------------------------------------------------------------===//
/// DbgVariableIntrinsic - This is the common base class for debug info
/// intrinsics for variables.
Index: llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
===================================================================
--- llvm/lib/CodeGen/PreISelIntrinsicLowering.cpp
+++ 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 bool lowerObjCCall(Function &F, const char *NewFn,
bool setNonLazyBind = false) {
+ assert(IntrinsicInst::mayLowerToFunctionCall(F.getIntrinsicID()) &&
+ "Pre-ISel intrinsics are lowered into regular function calls");
if (F.use_empty())
return false;
@@ -107,7 +110,9 @@
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
Index: llvm/include/llvm/IR/IntrinsicInst.h
===================================================================
--- llvm/include/llvm/IR/IntrinsicInst.h
+++ llvm/include/llvm/IR/IntrinsicInst.h
@@ -107,6 +107,9 @@
return false;
}
+ // Check if this intrinsic might lower into a regular function call
+ 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())
Index: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
===================================================================
--- /dev/null
+++ clang/test/CodeGenObjCXX/arc-exceptions-seh.mm
@@ -0,0 +1,23 @@
+// 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]]) ]
Index: clang/lib/CodeGen/CGCall.cpp
===================================================================
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4463,17 +4463,22 @@
// 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 and don't lower into regular function
+ // calls.
+ 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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
