[WinEH] Fix try scopes leaking to caller on inline #167176
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: Mirko (MuellerMP) ChangesThis fixes issue #164169 When inlining functions compiled with -EHa, try scope terminators might need to be inserted before inlined returns. This prevents leaking try scopes over to the caller. Try scopes can be ended before a ret due to the unwinder forwarding exceptions in the seh epilog to the caller. Patch is 30.62 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/167176.diff 8 Files Affected:
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 465f3f4e670c2..44a71b67e56bc 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -3982,63 +3982,41 @@ llvm::Value *CodeGenFunction::EmitCMSEClearRecord(llvm::Value *Src,
return R;
}
-void CodeGenFunction::EmitFunctionEpilog(
- const CGFunctionInfo &FI, bool EmitRetDbgLoc, SourceLocation EndLoc,
- uint64_t RetKeyInstructionsSourceAtom) {
- if (FI.isNoReturn()) {
- // Noreturn functions don't return.
- EmitUnreachable(EndLoc);
- return;
- }
-
- if (CurCodeDecl && CurCodeDecl->hasAttr<NakedAttr>()) {
- // Naked functions don't have epilogues.
- Builder.CreateUnreachable();
- return;
- }
-
- // Functions with no result always return void.
- if (!ReturnValue.isValid()) {
- auto *I = Builder.CreateRetVoid();
- if (RetKeyInstructionsSourceAtom)
- addInstToSpecificSourceAtom(I, nullptr, RetKeyInstructionsSourceAtom);
- else
- addInstToNewSourceAtom(I, nullptr);
- return;
- }
-
- llvm::DebugLoc RetDbgLoc;
- llvm::Value *RV = nullptr;
- QualType RetTy = FI.getReturnType();
+static void processFunctionReturnInfo(CodeGenFunction &CGF,
+ const CGFunctionInfo &FI,
+ bool EmitRetDbgLoc, SourceLocation EndLoc,
+ QualType RetTy, llvm::Value *&RV,
+ llvm::DebugLoc &RetDbgLoc) {
const ABIArgInfo &RetAI = FI.getReturnInfo();
switch (RetAI.getKind()) {
case ABIArgInfo::InAlloca:
// Aggregates get evaluated directly into the destination. Sometimes we
// need to return the sret value in a register, though.
- assert(hasAggregateEvaluationKind(RetTy));
+ assert(CodeGenFunction::hasAggregateEvaluationKind(RetTy));
if (RetAI.getInAllocaSRet()) {
- llvm::Function::arg_iterator EI = CurFn->arg_end();
+ llvm::Function::arg_iterator EI = CGF.CurFn->arg_end();
--EI;
llvm::Value *ArgStruct = &*EI;
- llvm::Value *SRet = Builder.CreateStructGEP(
+ llvm::Value *SRet = CGF.Builder.CreateStructGEP(
FI.getArgStruct(), ArgStruct, RetAI.getInAllocaFieldIndex());
llvm::Type *Ty =
cast<llvm::GetElementPtrInst>(SRet)->getResultElementType();
- RV = Builder.CreateAlignedLoad(Ty, SRet, getPointerAlign(), "sret");
+ RV = CGF.Builder.CreateAlignedLoad(Ty, SRet, CGF.getPointerAlign(),
+ "sret");
}
break;
case ABIArgInfo::Indirect: {
- auto AI = CurFn->arg_begin();
+ auto AI = CGF.CurFn->arg_begin();
if (RetAI.isSRetAfterThis())
++AI;
- switch (getEvaluationKind(RetTy)) {
+ switch (CodeGenFunction::getEvaluationKind(RetTy)) {
case TEK_Complex: {
- ComplexPairTy RT =
- EmitLoadOfComplex(MakeAddrLValue(ReturnValue, RetTy), EndLoc);
- EmitStoreOfComplex(RT, MakeNaturalAlignAddrLValue(&*AI, RetTy),
- /*isInit*/ true);
+ CodeGenFunction::ComplexPairTy RT = CGF.EmitLoadOfComplex(
+ CGF.MakeAddrLValue(CGF.ReturnValue, RetTy), EndLoc);
+ CGF.EmitStoreOfComplex(RT, CGF.MakeNaturalAlignAddrLValue(&*AI, RetTy),
+ /*isInit*/ true);
break;
}
case TEK_Aggregate:
@@ -4048,13 +4026,14 @@ void CodeGenFunction::EmitFunctionEpilog(
LValueBaseInfo BaseInfo;
TBAAAccessInfo TBAAInfo;
CharUnits Alignment =
- CGM.getNaturalTypeAlignment(RetTy, &BaseInfo, &TBAAInfo);
- Address ArgAddr(&*AI, ConvertType(RetTy), Alignment);
- LValue ArgVal =
- LValue::MakeAddr(ArgAddr, RetTy, getContext(), BaseInfo, TBAAInfo);
- EmitStoreOfScalar(
- EmitLoadOfScalar(MakeAddrLValue(ReturnValue, RetTy), EndLoc), ArgVal,
- /*isInit*/ true);
+ CGF.CGM.getNaturalTypeAlignment(RetTy, &BaseInfo, &TBAAInfo);
+ Address ArgAddr(&*AI, CGF.ConvertType(RetTy), Alignment);
+ LValue ArgVal = LValue::MakeAddr(ArgAddr, RetTy, CGF.getContext(),
+ BaseInfo, TBAAInfo);
+ CGF.EmitStoreOfScalar(
+ CGF.EmitLoadOfScalar(CGF.MakeAddrLValue(CGF.ReturnValue, RetTy),
+ EndLoc),
+ ArgVal, /*isInit*/ true);
break;
}
}
@@ -4063,37 +4042,37 @@ void CodeGenFunction::EmitFunctionEpilog(
case ABIArgInfo::Extend:
case ABIArgInfo::Direct:
- if (RetAI.getCoerceToType() == ConvertType(RetTy) &&
+ if (RetAI.getCoerceToType() == CGF.ConvertType(RetTy) &&
RetAI.getDirectOffset() == 0) {
// The internal return value temp always will have pointer-to-return-type
// type, just do a load.
// If there is a dominating store to ReturnValue, we can elide
// the load, zap the store, and usually zap the alloca.
- if (llvm::StoreInst *SI = findDominatingStoreToReturnValue(*this)) {
+ if (llvm::StoreInst *SI = findDominatingStoreToReturnValue(CGF)) {
// Reuse the debug location from the store unless there is
// cleanup code to be emitted between the store and return
// instruction.
- if (EmitRetDbgLoc && !AutoreleaseResult)
+ if (EmitRetDbgLoc && !CGF.AutoreleaseResult)
RetDbgLoc = SI->getDebugLoc();
// Get the stored value and nuke the now-dead store.
RV = SI->getValueOperand();
SI->eraseFromParent();
- // Otherwise, we have to do a simple load.
+ // Otherwise, we have to do a simple load.
} else {
- RV = Builder.CreateLoad(ReturnValue);
+ RV = CGF.Builder.CreateLoad(CGF.ReturnValue);
}
} else {
// If the value is offset in memory, apply the offset now.
- Address V = emitAddressAtOffset(*this, ReturnValue, RetAI);
+ Address V = emitAddressAtOffset(CGF, CGF.ReturnValue, RetAI);
- RV = CreateCoercedLoad(V, RetAI.getCoerceToType(), *this);
+ RV = CreateCoercedLoad(V, RetAI.getCoerceToType(), CGF);
}
// In ARC, end functions that return a retainable type with a call
// to objc_autoreleaseReturnValue.
- if (AutoreleaseResult) {
+ if (CGF.AutoreleaseResult) {
#ifndef NDEBUG
// Type::isObjCRetainabletype has to be called on a QualType that hasn't
// been stripped of the typedefs, so we cannot use RetTy here. Get the
@@ -4101,19 +4080,19 @@ void CodeGenFunction::EmitFunctionEpilog(
// CurCodeDecl or BlockInfo.
QualType RT;
- if (auto *FD = dyn_cast<FunctionDecl>(CurCodeDecl))
+ if (auto *FD = dyn_cast<FunctionDecl>(CGF.CurCodeDecl))
RT = FD->getReturnType();
- else if (auto *MD = dyn_cast<ObjCMethodDecl>(CurCodeDecl))
+ else if (auto *MD = dyn_cast<ObjCMethodDecl>(CGF.CurCodeDecl))
RT = MD->getReturnType();
- else if (isa<BlockDecl>(CurCodeDecl))
- RT = BlockInfo->BlockExpression->getFunctionType()->getReturnType();
+ else if (isa<BlockDecl>(CGF.CurCodeDecl))
+ RT = CGF.BlockInfo->BlockExpression->getFunctionType()->getReturnType();
else
llvm_unreachable("Unexpected function/method type");
- assert(getLangOpts().ObjCAutoRefCount && !FI.isReturnsRetained() &&
+ assert(CGF.getLangOpts().ObjCAutoRefCount && !FI.isReturnsRetained() &&
RT->isObjCRetainableType());
#endif
- RV = emitAutoreleaseOfResult(*this, RV);
+ RV = emitAutoreleaseOfResult(CGF, RV);
}
break;
@@ -4128,19 +4107,19 @@ void CodeGenFunction::EmitFunctionEpilog(
// Load all of the coerced elements out into results.
llvm::SmallVector<llvm::Value *, 4> results;
- Address addr = ReturnValue.withElementType(coercionType);
+ Address addr = CGF.ReturnValue.withElementType(coercionType);
unsigned unpaddedIndex = 0;
for (unsigned i = 0, e = coercionType->getNumElements(); i != e; ++i) {
auto coercedEltType = coercionType->getElementType(i);
if (ABIArgInfo::isPaddingForCoerceAndExpand(coercedEltType))
continue;
- auto eltAddr = Builder.CreateStructGEP(addr, i);
+ auto eltAddr = CGF.Builder.CreateStructGEP(addr, i);
llvm::Value *elt = CreateCoercedLoad(
eltAddr,
unpaddedStruct ? unpaddedStruct->getElementType(unpaddedIndex++)
: unpaddedCoercionType,
- *this);
+ CGF);
results.push_back(elt);
}
@@ -4148,27 +4127,58 @@ void CodeGenFunction::EmitFunctionEpilog(
if (results.size() == 1) {
RV = results[0];
- // Otherwise, we need to make a first-class aggregate.
+ // Otherwise, we need to make a first-class aggregate.
} else {
// Construct a return type that lacks padding elements.
llvm::Type *returnType = RetAI.getUnpaddedCoerceAndExpandType();
RV = llvm::PoisonValue::get(returnType);
for (unsigned i = 0, e = results.size(); i != e; ++i) {
- RV = Builder.CreateInsertValue(RV, results[i], i);
+ RV = CGF.Builder.CreateInsertValue(RV, results[i], i);
}
}
break;
}
case ABIArgInfo::TargetSpecific: {
- Address V = emitAddressAtOffset(*this, ReturnValue, RetAI);
- RV = CGM.getABIInfo().createCoercedLoad(V, RetAI, *this);
+ Address V = emitAddressAtOffset(CGF, CGF.ReturnValue, RetAI);
+ RV = CGF.CGM.getABIInfo().createCoercedLoad(V, RetAI, CGF);
break;
}
case ABIArgInfo::Expand:
case ABIArgInfo::IndirectAliased:
llvm_unreachable("Invalid ABI kind for return argument");
}
+}
+
+static bool isReturnReachable(CodeGenFunction::JumpDest &ReturnBlock) {
+ return !ReturnBlock.isValid() || !ReturnBlock.getBlock()->use_empty();
+}
+
+void CodeGenFunction::EmitFunctionEpilog(
+ const CGFunctionInfo &FI, bool EmitRetDbgLoc, SourceLocation EndLoc,
+ uint64_t RetKeyInstructionsSourceAtom) {
+ if (FI.isNoReturn()) {
+ // Noreturn functions don't return.
+ EmitUnreachable(EndLoc);
+ return;
+ }
+
+ if (CurCodeDecl && CurCodeDecl->hasAttr<NakedAttr>()) {
+ // Naked functions don't have epilogues.
+ Builder.CreateUnreachable();
+ return;
+ }
+
+ llvm::Value *RV = nullptr;
+ llvm::DebugLoc RetDbgLoc;
+ QualType RetTy = FI.getReturnType();
+
+ if (ReturnValue.isValid())
+ processFunctionReturnInfo(*this, FI, EmitRetDbgLoc, EndLoc, RetTy, RV,
+ RetDbgLoc);
+
+ if (SehTryEndInvokeDest && isReturnReachable(ReturnBlock))
+ EmitSehTryScopeEnd(SehTryEndInvokeDest);
llvm::Instruction *Ret;
if (RV) {
@@ -4203,7 +4213,7 @@ void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV) {
// If the return block isn't reachable, neither is this check, so don't emit
// it.
- if (ReturnBlock.isValid() && ReturnBlock.getBlock()->use_empty())
+ if (!isReturnReachable(ReturnBlock))
return;
ReturnsNonNullAttr *RetNNAttr = nullptr;
diff --git a/clang/lib/CodeGen/CGCleanup.cpp b/clang/lib/CodeGen/CGCleanup.cpp
index 28ac9bf396356..6d3bbd672517a 100644
--- a/clang/lib/CodeGen/CGCleanup.cpp
+++ b/clang/lib/CodeGen/CGCleanup.cpp
@@ -1329,8 +1329,10 @@ void CodeGenFunction::EmitCXXTemporary(const CXXTemporary *Temporary,
// Need to set "funclet" in OperandBundle properly for noThrow
// intrinsic (see CGCall.cpp)
static void EmitSehScope(CodeGenFunction &CGF,
- llvm::FunctionCallee &SehCppScope) {
- llvm::BasicBlock *InvokeDest = CGF.getInvokeDest();
+ llvm::FunctionCallee &SehCppScope,
+ llvm::BasicBlock* InvokeDest = nullptr) {
+ if (!InvokeDest)
+ InvokeDest = CGF.getInvokeDest();
assert(CGF.Builder.GetInsertBlock() && InvokeDest);
llvm::BasicBlock *Cont = CGF.createBasicBlock("invoke.cont");
SmallVector<llvm::OperandBundleDef, 1> BundleList =
@@ -1373,11 +1375,11 @@ void CodeGenFunction::EmitSehTryScopeBegin() {
}
// Invoke a llvm.seh.try.end at the end of a SEH scope for -EHa
-void CodeGenFunction::EmitSehTryScopeEnd() {
+void CodeGenFunction::EmitSehTryScopeEnd(llvm::BasicBlock *InvokeDest) {
assert(getLangOpts().EHAsynch);
llvm::FunctionType *FTy =
llvm::FunctionType::get(CGM.VoidTy, /*isVarArg=*/false);
llvm::FunctionCallee SehCppScope =
CGM.CreateRuntimeFunction(FTy, "llvm.seh.try.end");
- EmitSehScope(*this, SehCppScope);
+ EmitSehScope(*this, SehCppScope, InvokeDest);
}
diff --git a/clang/lib/CodeGen/CGException.cpp b/clang/lib/CodeGen/CGException.cpp
index f86af4581c345..bc02264b4a58b 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -635,6 +635,37 @@ void CodeGenFunction::EmitCXXTryStmt(const CXXTryStmt &S) {
ExitCXXTryStmt(S);
}
+static bool
+RequiresSehTryEnd(const CompoundStmt* S,
+ llvm::SmallPtrSet<const Stmt *, 1> &CheckedSehTryBlockStmts) {
+ if (!S || CheckedSehTryBlockStmts.contains(S))
+ return false;
+
+ llvm::SmallVector<const Stmt *> WorkList;
+ WorkList.push_back(S);
+
+ while (!WorkList.empty()) {
+ auto *Next = WorkList.back();
+ WorkList.pop_back();
+ if (!Next)
+ continue;
+
+ if (isa<ReturnStmt>(Next))
+ return true;
+
+ if (auto *Try = dyn_cast<CXXTryStmt>(Next))
+ CheckedSehTryBlockStmts.insert(Try->getTryBlock());
+
+ if (auto *Try = dyn_cast<SEHTryStmt>(Next))
+ CheckedSehTryBlockStmts.insert(Try->getTryBlock());
+
+ auto Children = Next->children();
+ WorkList.insert(WorkList.end(), Children.begin(), Children.end());
+ }
+
+ return false;
+}
+
void CodeGenFunction::EnterCXXTryStmt(const CXXTryStmt &S, bool IsFnTryBlock) {
unsigned NumHandlers = S.getNumHandlers();
EHCatchScope *CatchScope = EHStack.pushCatch(NumHandlers);
@@ -666,8 +697,14 @@ void CodeGenFunction::EnterCXXTryStmt(const CXXTryStmt &S, bool IsFnTryBlock) {
CatchScope->setHandler(I, CGM.getCXXABI().getCatchAllTypeInfo(), Handler);
// Under async exceptions, catch(...) need to catch HW exception too
// Mark scope with SehTryBegin as a SEH __try scope
- if (getLangOpts().EHAsynch)
+ if (getLangOpts().EHAsynch) {
EmitSehTryScopeBegin();
+ auto *TryBlock = S.getTryBlock();
+
+ if (!SehTryEndInvokeDest &&
+ RequiresSehTryEnd(TryBlock, CheckedSehTryBlockStmts))
+ SehTryEndInvokeDest = getInvokeDest();
+ }
}
}
}
@@ -1670,14 +1707,18 @@ void CodeGenFunction::EmitSEHTryStmt(const SEHTryStmt &S) {
SEHTryEpilogueStack.push_back(&TryExit);
llvm::BasicBlock *TryBB = nullptr;
+ auto *TryBlock = S.getTryBlock();
// IsEHa: emit an invoke to _seh_try_begin() runtime for -EHa
if (getLangOpts().EHAsynch) {
EmitRuntimeCallOrInvoke(getSehTryBeginFn(CGM));
if (SEHTryEpilogueStack.size() == 1) // outermost only
TryBB = Builder.GetInsertBlock();
+ if (!SehTryEndInvokeDest &&
+ RequiresSehTryEnd(TryBlock, CheckedSehTryBlockStmts))
+ SehTryEndInvokeDest = getInvokeDest();
}
- EmitStmt(S.getTryBlock());
+ EmitStmt(TryBlock);
// Volatilize all blocks in Try, till current insert point
if (TryBB) {
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 8c4c1c8c2dc95..1920e8d562674 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -2129,6 +2129,12 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Terminate funclets keyed by parent funclet pad.
llvm::MapVector<llvm::Value *, llvm::BasicBlock *> TerminateFunclets;
+ /// Visited try block statements that do not need a scope end.
+ llvm::SmallPtrSet<const Stmt *, 1> CheckedSehTryBlockStmts;
+
+ /// Unwind destination for try scope end.
+ llvm::BasicBlock *SehTryEndInvokeDest = nullptr;
+
/// Largest vector width used in ths function. Will be used to create a
/// function attribute.
unsigned LargestVectorWidth = 0;
@@ -3241,7 +3247,7 @@ class CodeGenFunction : public CodeGenTypeCache {
void EmitSehCppScopeBegin();
void EmitSehCppScopeEnd();
void EmitSehTryScopeBegin();
- void EmitSehTryScopeEnd();
+ void EmitSehTryScopeEnd(llvm::BasicBlock *InvokeDest = nullptr);
bool EmitLifetimeStart(llvm::Value *Addr);
void EmitLifetimeEnd(llvm::Value *Addr);
diff --git a/clang/test/CodeGen/windows-seh-EHa-Inline1.cpp b/clang/test/CodeGen/windows-seh-EHa-Inline1.cpp
new file mode 100644
index 0000000000000..26d8cc204274e
--- /dev/null
+++ b/clang/test/CodeGen/windows-seh-EHa-Inline1.cpp
@@ -0,0 +1,62 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 6
+// RUN: %clang_cc1 -O3 -triple x86_64-windows -fasync-exceptions -fcxx-exceptions -fexceptions -fms-extensions -x c++ -Wno-implicit-function-declaration -emit-llvm %s -o - | FileCheck %s
+// Check that the try scope of ExitOnThrow is terminated upon inlining into main.
+int AlwaysThrows(int);
+[[noreturn]] void Exit();
+
+int ExitOnThrow(int argc) noexcept
+{
+ try {
+ if (!argc) { throw -1; }
+ return argc;
+ } catch (...) {
+ }
+
+ Exit();
+ return 0;
+}
+
+// CHECK-LABEL: define dso_local noundef i32 @main(
+// CHECK-SAME: i32 noundef [[ARGC:%.*]], ptr noundef readnone captures(none) [[TMP0:%.*]]) local_unnamed_addr #[[ATTR3:[0-9]+]] personality ptr @__CxxFrameHandler3 {
+// CHECK-NEXT: [[ENTRY:.*:]]
+// CHECK-NEXT: [[TMP_I:%.*]] = alloca i32, align 4
+// CHECK-NEXT: call void @llvm.lifetime.start.p0(ptr nonnull [[TMP_I]])
+// CHECK-NEXT: invoke void @llvm.seh.try.begin()
+// CHECK-NEXT: to label %[[INVOKE_CONT_I:.*]] unwind label %[[CATCH_DISPATCH_I:.*]]
+// CHECK: [[INVOKE_CONT_I]]:
+// CHECK-NEXT: [[TOBOOL_NOT_I:%.*]] = icmp eq i32 [[ARGC]], 0
+// CHECK-NEXT: br i1 [[TOBOOL_NOT_I]], label %[[IF_THEN_I:.*]], label %[[IF_END_I:.*]]
+// CHECK: [[IF_THEN_I]]:
+// CHECK-NEXT: store i32 -1, ptr [[TMP_I]], align 4, !tbaa [[INT_TBAA7:![0-9]+]]
+// CHECK-NEXT: invoke void @_CxxThrowException(ptr nonnull [[TMP_I]], ptr nonnull @_TI1H) #[[ATTR6:[0-9]+]]
+// CHECK-NEXT: to label %[[UNREACHABLE_I:.*]] unwind label %[[CATCH_DISPATCH_I]]
+// CHECK: [[CATCH_DISPATCH_I]]:
+// CHECK-NEXT: [[TMP1:%.*]] = catchswitch within none [label %[[CATCH_I:.*]]] unwind to caller
+// CHECK: [[CATCH_I]]:
+// CHECK-NEXT: [[TMP2:%.*]] = catchpad within [[TMP1]] [ptr null, i32 0, ptr null]
+// CHECK-NEXT: catchret from [[TMP2]] to label %[[TRY_CONT_I:.*]]
+// CHECK: [[TRY_CONT_I]]:
+// CHECK-NEXT: call void @"?Exit@@YAXXZ"() #[[ATTR7:[0-9]+]]
+// CHECK-NEXT: unreachable
+// CHECK: [[IF_END_I]]:
+// CHECK-NEXT: invoke void @llvm.seh.try.end()
+// CHECK-NEXT: to label %"?ExitOnThrow@@[email protected]" unwind label %[[CATCH_DISPATCH_I]]
+// CHECK: [[UNREACHABLE_I]]:
+// CHECK-NEXT: unreachable
+// CHECK: "?ExitOnThrow@@[email protected]":
+// CHECK-NEXT: call void @llvm.lifetime.end.p0(ptr nonnull [[TMP_I]])
+// CHECK-NEXT: [[CALL1:%.*]] = tail call noundef i32 @"?AlwaysThrows@@YAHH@Z"(i32 noundef [[ARGC]])
+// CHECK-NEXT: [[ADD:%.*]] = add nsw i32 [[CALL1]], [[ARGC]]
+// CHECK-NEXT: ret i32 [[ADD]]
+//
+int main(int argc, char**)
+{
+ auto data = ExitOnThrow(argc);
+ return data + AlwaysThrows(data);
+}
+//.
+// CHECK: [[INT_TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0}
+// CHECK: [[META8]] = !{!"int", [[META9:![0-9]+]], i64 0}
+// CHECK: [[META9]] = !{!"omnipotent char", [[META10:![0-9]+]], i64 0}
+// CHECK: [[META10]] = !{!"Simple C++ TBAA"}
+//.
diff --git a/clang/test/CodeGen/windows-seh-EHa-Inline2.cpp b/clang/test/CodeGen/windows-seh-EHa-Inline2.cpp
new file mode 100644
index 0000000000000..f8539cb315dfb
--- /dev/null
+++ b/clang/test/CodeGen/windows-seh-EHa-Inline2.cpp
@@ -0,0 +1,103 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 6
+// RUN: %clang_cc1 -O3 -triple x86_64-windows -fasync-exceptions -fcxx-exceptions -fexceptions -fms-extensions -x c++ -Wno-implicit-function-declaration -emit-llvm %s -o - | FileCheck %s
+// Check that only the outermost try scope containing a return statement is terminated upon inlining into main.
+void DoSth();
+int AlwaysThrows(int);
+[[noreturn]] void Exit();
+
+int ExitOnThrow(int argc) noexcept
+{
+ try {
+ try {
+ DoSth();
+ } catch(...) {}
+ } catch(...) {}
+
+ try {
+ try {
+ if (!argc) { throw -1; }
+ retu...
[truncated]
|
Contributor
Author
|
This is a follow up to #164170 for reference |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/CodeGen/CGCall.cpp
Outdated
| RetDbgLoc); | ||
|
|
||
| if (SehTryEndInvokeDest && isReturnReachable(ReturnBlock)) | ||
| EmitSehTryScopeEnd(SehTryEndInvokeDest); |
Collaborator
There was a problem hiding this comment.
This looks wrong. You need to end the scope in the ReturnStmt, before you branch to the return block. The return block is shared by all return statements; different return statements can be in different scopes.
CodeGenFunction::EmitReturnStmt calls EmitBranchThroughCleanup, which should pop the cleanup scopes, and as part of that should call EmitSehTryScopeEnd. I haven't dug into EmitBranchThroughCleanup to figure out why that isn't working the way it's supposed to.
Contributor
Author
There was a problem hiding this comment.
I think it is possible to formulate this as a NormalCleanup on the EHStack (similar how SEH finally scopes are pushed). I initially put this here (and fixed up WinEHPrepare to work with already ended scopes) so i can emit the scope end just before the ret and keep return value loads inside the try scope.
But I do agree that using the EHStack is probably the better approach here.
Contributor
Author
There was a problem hiding this comment.
Made some progress on this - the only annoying part seems to be that i want the InvokeDest for my try scope terminator preferably before the CatchScope is pushed.
My current workaround is that I cache the outermost invokedest in SehTryEndInvokeDest.
MuellerMP
force-pushed
the
main
branch
2 times, most recently
from
8d52900 to
1ef74fa
Compare
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
MuellerMP
force-pushed
the
main
branch
2 times, most recently
from
cf53250 to
46646ed
Compare
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
This seems like basically the right approach.
llvm/lib/CodeGen/WinEHPrepare.cpp
Outdated
| State = EHInfo.InvokeStateMap[cast<InvokeInst>(TI)]; | ||
| else if (Fn && Fn->isIntrinsic() && | ||
| Fn->getIntrinsicID() == Intrinsic::seh_try_end) | ||
| Fn->getIntrinsicID() == Intrinsic::seh_try_end) { |
Collaborator
There was a problem hiding this comment.
If there's an issue with the backend, it needs its own test, with an LLVM IR input file.
Contributor
Author
There was a problem hiding this comment.
This reuses the invoke state map for try.end in a fashion already available for the cxx state numbering algorithm.
It allows to terminate multiple SEH scopes at once without needing an additional cleanup scope per nested try.
It works by looking up the state for the matching unwind destination and then decrements the state number (which is expressed by the ToState assignment).
I added a nested try catch test which showcases this (without this patch we will end up in Exit instead of having the exception propagate to the caller).
Collaborator
There was a problem hiding this comment.
Terminating multiple scopes at once is fine, I guess, as an optimization. But I'm a little concerned that indicates the frontend doesn't understand which scope it's leaving.
Contributor
Author
There was a problem hiding this comment.
Updated my patch: the current patch now reverts back to emitting a try end for every eha try. The related invoke destination is now also present in the TerminateTryScope cleanup struct.
Contributor
Author
clang/lib/CodeGen/CGException.cpp
Outdated
| if (SEHTryEpilogueStack.size() == 1) // outermost only | ||
| TryBB = Builder.GetInsertBlock(); | ||
| if (!SehTryEndInvokeDest) | ||
| SehTryEndInvokeDest = getInvokeDest(); |
Collaborator
There was a problem hiding this comment.
I'd prefer to store the destination alongside its corresponding cleanup, instead of sticking in a member of CodeGenFunction. Having it stored like this makes the interaction with nesting confusing.
Contributor
Author
There was a problem hiding this comment.
This is the ugly part about my current patch. I have yet to find a good solution to get the InvokeDest before the pushCatch.
I don't think mutating the early pushed TerminateTryScope is a good way to solve things since it will require to reinterpret_cast the cleanupbuffer which is not intended I suppose.
I think the best solution would probably be to generate the LandingPad in advance (basically what getInvokeDestImpl does - but without the Catch being present on the stack).
Contributor
Author
There was a problem hiding this comment.
Update: My current solution is now to just create the unwind destination basicblock early fo the TerminateTryScope instances which i can cache with setCachedEHDispatchBlock.
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Not sure when @rnk will have a chance to respond; let's try to move forward without him.
|
|
||
| @p = dso_local global ptr null, align 8 | ||
|
|
||
| define dso_local noundef i32 @main(i32 noundef %argc, ptr noundef readnone captures(none) %argv) local_unnamed_addr personality ptr @__C_specific_handler { |
Collaborator
There was a problem hiding this comment.
If this was originally C++, maybe put the corresponding C++ code for reference.
Contributor
Author
There was a problem hiding this comment.
I updated this test and added the c code for it.
It does not test the termination of multiple scopes at once anymore.
Instead it now tests the bugfix in the seh state numbering: the added State >= 0 condition in WinEHPrepare.
llvm/lib/CodeGen/WinEHPrepare.cpp
Outdated
| State = EHInfo.InvokeStateMap[cast<InvokeInst>(TI)]; | ||
| else if (Fn && Fn->isIntrinsic() && | ||
| Fn->getIntrinsicID() == Intrinsic::seh_try_end) | ||
| Fn->getIntrinsicID() == Intrinsic::seh_try_end) { |
Collaborator
There was a problem hiding this comment.
Terminating multiple scopes at once is fine, I guess, as an optimization. But I'm a little concerned that indicates the frontend doesn't understand which scope it's leaving.
This fixes issue llvm#164169 When inlining functions compiled with -EHa, try scope terminators might need to be inserted before inlined returns. This prevents leaking try scopes over to the caller. Try scopes can be ended before a ret due to the unwinder forwarding exceptions in the seh epilog to the caller.
Collaborator
efriedma-quic
left a comment
efriedma-quic
left a comment
There was a problem hiding this comment.
Does this fix #62723?
| unsigned NumHandlerInfos = NumHandlers > 0 ? NumHandlers - 1 : 0; | ||
| llvm::BasicBlock *DispatchBlock = nullptr; | ||
| llvm::SmallVector<HandlerInfo> HandlerInfos; | ||
| HandlerInfos.reserve(NumHandlerInfos); |
Collaborator
There was a problem hiding this comment.
It's not worth the extra code here to try to reserve the exact amount in the array.
| @@ -0,0 +1,62 @@ | |||
| // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 6 | |||
| // RUN: %clang_cc1 -O3 -triple x86_64-windows -fasync-exceptions -fcxx-exceptions -fexceptions -fms-extensions -x c++ -Wno-implicit-function-declaration -emit-llvm %s -o - | FileCheck %s | |||
Collaborator
There was a problem hiding this comment.
We generally prefer clang tests to run with optimizations disabled: it makes it easier to see the correspondence between what we're testing and what's actually getting generated, and it makes the tests less sensitive to optimizer changes. (There's a minor downside that it doesn't always capture the end-to-end result, but usually a set of clang tests plus a set of LLVM optimizer tests is sufficient.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes issue #164169
When inlining functions compiled with -EHa, try scope terminators might need to be inserted before inlined returns. This prevents leaking try scopes over to the caller. Try scopes can be ended before a ret due to the unwinder forwarding exceptions in the seh epilog to the caller.