From a2ed036c0ae2522747732da4815e46c45fb89ac5 Mon Sep 17 00:00:00 2001 From: David Majnemer Date: Fri, 22 Jan 2016 23:20:43 +0000 Subject: [PATCH] [WinEH] Let cleanups post-dominated by unreachable get executed Cleanups in C++ are a little weird. They are only guaranteed to be reliably executed if, and only if, there is a viable catch handler which can handle the exception. This means that reachability of a cleanup is lexically determined by it being nested with a try-block which unwinds to a catch. It is *cannot* be reasoned about by examining the control flow edges leaving a cleanup. Usually this is not a problem. It becomes a problem when there are *no* edges out of a cleanup because we believed that code post-dominated by the cleanup is dead. In LLVM's case, this code is what informs the personality routine about the presence of a suitable catch handler. However, the lack of edges to that catch handler makes the handler become unreachable which causes us to remove it. By removing the handler, the cleanup becomes unreachable. Instead, inject a catch-all handler with every cleanup that has no unwind edges. This will allow us to properly unwind the stack. This fixes PR25997. llvm-svn: 258580 --- lib/CodeGen/WinEHPrepare.cpp | 112 ++++++++++++++++++ .../WinEH/wineh-cleanuppad-nounwind.ll | 71 +++++++++++ test/CodeGen/WinEH/wineh-cloning.ll | 4 +- test/CodeGen/WinEH/wineh-demotion.ll | 3 +- test/CodeGen/WinEH/wineh-no-demotion.ll | 2 +- 5 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 test/CodeGen/WinEH/wineh-cleanuppad-nounwind.ll diff --git a/lib/CodeGen/WinEHPrepare.cpp b/lib/CodeGen/WinEHPrepare.cpp index 281fe934292..c4eaef88673 100644 --- a/lib/CodeGen/WinEHPrepare.cpp +++ b/lib/CodeGen/WinEHPrepare.cpp @@ -81,6 +81,7 @@ private: void cloneCommonBlocks(Function &F); void removeImplausibleInstructions(Function &F); void cleanupPreparedFunclets(Function &F); + void fixupNoReturnCleanupPads(Function &F); void verifyPreparedFunclets(Function &F); // All fields are reset by runOnFunction. @@ -656,6 +657,7 @@ void llvm::calculateClrEHStateNumbers(const Function *Fn, void WinEHPrepare::colorFunclets(Function &F) { BlockColors = colorEHFunclets(F); + FuncletBlocks.clear(); // Invert the map from BB to colors to color to BBs. for (BasicBlock &BB : F) { @@ -1001,6 +1003,114 @@ void WinEHPrepare::cleanupPreparedFunclets(Function &F) { removeUnreachableBlocks(F); } +// Cleanuppads which are postdominated by unreachable will not unwind to any +// catchswitches, making them dead. This is problematic if the original source +// had a catch clause which could legitimately catch the exception, causing the +// cleanup to run. +// +// This is only a problem for C++ where down-stream catches cause cleanups to +// run. +void WinEHPrepare::fixupNoReturnCleanupPads(Function &F) { + // We only need to do this for C++ personality routines, + // skip this work for all others. + if (Personality != EHPersonality::MSVC_CXX) + return; + + // Do a quick sanity check on the color map before we throw it away so as to + // avoid hiding latent bugs. + DEBUG(verifyPreparedFunclets(F)); + // Re-color the funclets because cleanupPreparedFunclets might have + // invalidated FuncletBlocks. + colorFunclets(F); + + // We create a unique catchswitch for each parent it will be nested within. + SmallDenseMap NewCatchSwitches; + // Create a new catchswitch+catchpad where the catchpad is post-dominated by + // unreachable. + auto GetOrCreateCatchSwitch = [&](Value *ParentPad) { + auto &CatchSwitch = NewCatchSwitches[ParentPad]; + if (CatchSwitch) + return CatchSwitch; + + auto *ParentBB = isa(ParentPad) + ? &F.getEntryBlock() + : cast(ParentPad)->getParent(); + + StringRef NameSuffix = ParentBB->getName(); + + BasicBlock *CatchSwitchBB = BasicBlock::Create( + F.getContext(), Twine("catchswitchbb.for.", NameSuffix)); + CatchSwitchBB->insertInto(&F, ParentBB->getNextNode()); + CatchSwitch = CatchSwitchInst::Create(ParentPad, /*UnwindDest=*/nullptr, + /*NumHandlers=*/1, + Twine("catchswitch.for.", NameSuffix), + CatchSwitchBB); + + BasicBlock *CatchPadBB = BasicBlock::Create( + F.getContext(), Twine("catchpadbb.for.", NameSuffix)); + CatchPadBB->insertInto(&F, CatchSwitchBB->getNextNode()); + Value *CatchPadArgs[] = { + Constant::getNullValue(Type::getInt8PtrTy(F.getContext())), + ConstantInt::get(Type::getInt32Ty(F.getContext()), 64), + Constant::getNullValue(Type::getInt8PtrTy(F.getContext())), + }; + CatchPadInst::Create(CatchSwitch, CatchPadArgs, + Twine("catchpad.for.", NameSuffix), CatchPadBB); + new UnreachableInst(F.getContext(), CatchPadBB); + + CatchSwitch->addHandler(CatchPadBB); + + return CatchSwitch; + }; + + // Look for all basic blocks which are within cleanups which are postdominated + // by unreachable. + for (auto &Funclets : FuncletBlocks) { + BasicBlock *FuncletPadBB = Funclets.first; + auto *CleanupPad = dyn_cast(FuncletPadBB->getFirstNonPHI()); + // Skip over any non-cleanup funclets. + if (!CleanupPad) + continue; + // Skip over any cleanups have unwind targets, they do not need this. + if (getCleanupRetUnwindDest(CleanupPad) != nullptr) + continue; + // Walk the blocks within the cleanup which end in 'unreachable'. + // We will replace the unreachable instruction with a cleanupret; + // this cleanupret will unwind to a catchswitch with a lone catch-all + // catchpad. + std::vector &BlocksInFunclet = Funclets.second; + for (BasicBlock *BB : BlocksInFunclet) { + auto *UI = dyn_cast(BB->getTerminator()); + if (!UI) + continue; + // Remove the unreachable instruction. + UI->eraseFromParent(); + + // Add our new cleanupret. + auto *ParentPad = CleanupPad->getParentPad(); + CatchSwitchInst *CatchSwitch = GetOrCreateCatchSwitch(ParentPad); + CleanupReturnInst::Create(CleanupPad, CatchSwitch->getParent(), BB); + } + } + + // Update BlockColors and FuncletBlocks to maintain WinEHPrepare's + // invariants. + for (auto CatchSwitchKV : NewCatchSwitches) { + CatchSwitchInst *CatchSwitch = CatchSwitchKV.second; + BasicBlock *CatchSwitchBB = CatchSwitch->getParent(); + + assert(CatchSwitch->getNumSuccessors() == 1); + BasicBlock *CatchPadBB = CatchSwitch->getSuccessor(0); + assert(isa(CatchPadBB->getFirstNonPHI())); + + BlockColors.insert({CatchSwitchBB, ColorVector(CatchSwitchBB)}); + FuncletBlocks[CatchSwitchBB] = {CatchSwitchBB}; + + BlockColors.insert({CatchPadBB, ColorVector(CatchPadBB)}); + FuncletBlocks[CatchPadBB] = {CatchPadBB}; + } +} + void WinEHPrepare::verifyPreparedFunclets(Function &F) { for (BasicBlock &BB : F) { size_t NumColors = BlockColors[&BB].size(); @@ -1036,6 +1146,8 @@ bool WinEHPrepare::prepareExplicitEH(Function &F) { cleanupPreparedFunclets(F); } + fixupNoReturnCleanupPads(F); + DEBUG(verifyPreparedFunclets(F)); // Recolor the CFG to verify that all is well. DEBUG(colorFunclets(F)); diff --git a/test/CodeGen/WinEH/wineh-cleanuppad-nounwind.ll b/test/CodeGen/WinEH/wineh-cleanuppad-nounwind.ll new file mode 100644 index 00000000000..09b4d3b8a1b --- /dev/null +++ b/test/CodeGen/WinEH/wineh-cleanuppad-nounwind.ll @@ -0,0 +1,71 @@ +; RUN: opt -S -winehprepare < %s | FileCheck %s +target triple = "x86_64-pc-windows-msvc" + +; CHECK-LABEL: @test1( +define void @test1(i1 %b) personality i32 (...)* @__CxxFrameHandler3 { +entry: + invoke void @f() + to label %try.cont unwind label %cleanup.bb + +; CHECK: entry: + +; CHECK: [[catchswitch_entry:.*]]: +; CHECK-NEXT: %[[cs0:.*]] = catchswitch within none [label %[[catchpad:.*]]] unwind to caller +; CHECK: [[catchpad]]: +; CHECK-NEXT: %[[cp0:.*]] = catchpad within %[[cs0]] [i8* null, i32 64, i8* null] +; CHECK-NEXT: unreachable + +try.cont: + invoke void @f() + to label %exit unwind label %catchswitch.bb + +cleanup.bb: + %cleanup = cleanuppad within none [] + br i1 %b, label %left, label %right + +left: + call void @exit(i32 0) [ "funclet"(token %cleanup) ] + unreachable + +right: + call void @exit(i32 1) [ "funclet"(token %cleanup) ] + unreachable + +catchswitch.bb: + %cs = catchswitch within none [label %catchpad.bb] unwind to caller + +; CHECK: catchpad.bb: +; CHECK-NEXT: %catch = catchpad within %cs [i8* null, i32 64, i8* null] + +; CHECK: [[catchswitch_catch:.*]]: +; CHECK-NEXT: %[[cs1:.*]] = catchswitch within %catch [label %[[catchpad_catch:.*]]] unwind to caller +; CHECK: [[catchpad_catch]]: +; CHECK-NEXT: %[[cp1:.*]] = catchpad within %[[cs1]] [i8* null, i32 64, i8* null] +; CHECK-NEXT: unreachable + +; CHECK: nested.cleanup.bb: +; CHECK-NEXT: %nested.cleanup = cleanuppad within %catch [] +; CHECK-NEXT: call void @exit(i32 2) [ "funclet"(token %nested.cleanup) ] +; CHECK-NEXT: cleanupret from %nested.cleanup unwind label %[[catchswitch_catch]] + +catchpad.bb: + %catch = catchpad within %cs [i8* null, i32 64, i8* null] + invoke void @f() [ "funclet"(token %catch) ] + to label %unreachable unwind label %nested.cleanup.bb + +nested.cleanup.bb: + %nested.cleanup = cleanuppad within %catch [] + call void @exit(i32 2) [ "funclet"(token %nested.cleanup) ] + unreachable + +unreachable: + unreachable + +exit: + unreachable +} + +declare void @f() +declare void @exit(i32) nounwind noreturn + +declare i32 @__CxxFrameHandler3(...) diff --git a/test/CodeGen/WinEH/wineh-cloning.ll b/test/CodeGen/WinEH/wineh-cloning.ll index 748c07df173..355ffa8c3f6 100644 --- a/test/CodeGen/WinEH/wineh-cloning.ll +++ b/test/CodeGen/WinEH/wineh-cloning.ll @@ -44,7 +44,7 @@ noreturn: ; CHECK: call void @llvm.foo(i32 %x) -define void @test2() personality i32 (...)* @__CxxFrameHandler3 { +define void @test2() personality i32 (...)* @__C_specific_handler { entry: invoke void @f() to label %exit unwind label %cleanup @@ -71,7 +71,7 @@ exit: ; CHECK-NEXT: ret void -define void @test3() personality i32 (...)* @__CxxFrameHandler3 { +define void @test3() personality i32 (...)* @__C_specific_handler { entry: invoke void @f() to label %invoke.cont unwind label %catch.switch diff --git a/test/CodeGen/WinEH/wineh-demotion.ll b/test/CodeGen/WinEH/wineh-demotion.ll index 411952d84bb..048466a26d1 100644 --- a/test/CodeGen/WinEH/wineh-demotion.ll +++ b/test/CodeGen/WinEH/wineh-demotion.ll @@ -1,6 +1,7 @@ ; RUN: opt -mtriple=x86_64-pc-windows-msvc -S -winehprepare < %s | FileCheck %s declare i32 @__CxxFrameHandler3(...) +declare i32 @__C_specific_handler(...) declare void @f() @@ -327,7 +328,7 @@ exit: } ; CHECK-LABEL: @test8( -define void @test8() personality i32 (...)* @__CxxFrameHandler3 { entry: +define void @test8() personality i32 (...)* @__C_specific_handler { entry: invoke void @f() to label %done unwind label %cleanup1 invoke void @f() diff --git a/test/CodeGen/WinEH/wineh-no-demotion.ll b/test/CodeGen/WinEH/wineh-no-demotion.ll index 0901e27c301..277dd5c0e6a 100644 --- a/test/CodeGen/WinEH/wineh-no-demotion.ll +++ b/test/CodeGen/WinEH/wineh-no-demotion.ll @@ -1,4 +1,4 @@ -; RUN: opt -mtriple=x86_x64-pc-windows-msvc -S -winehprepare -disable-demotion -disable-cleanups < %s | FileCheck %s +; RUN: opt -mtriple=x86_64-pc-windows-msvc -S -winehprepare -disable-demotion -disable-cleanups < %s | FileCheck %s declare i32 @__CxxFrameHandler3(...)