Skip to content

[Clang] Fixed double finally block execution #146796

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[Clang] Fixed double finally block execution #146796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ccf384
Fixed double finally block execution
Jul 1, 2025
b2da5a7
Merge branch 'main' into user/mmissoum/cg-double-finally-fix
yasster Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions clang/lib/CodeGen/CGException.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1368,14 +1368,24 @@ namespace {
llvm::FunctionCallee EndCatchFn;
llvm::FunctionCallee RethrowFn;
llvm::Value *SavedExnVar;
llvm::Value *FinallyExecutedFlag;

PerformFinally(const Stmt *Body, llvm::Value *ForEHVar,
llvm::FunctionCallee EndCatchFn,
llvm::FunctionCallee RethrowFn, llvm::Value *SavedExnVar)
llvm::FunctionCallee RethrowFn, llvm::Value *SavedExnVar,
llvm::Value *FinallyExecutedFlag)
: Body(Body), ForEHVar(ForEHVar), EndCatchFn(EndCatchFn),
RethrowFn(RethrowFn), SavedExnVar(SavedExnVar) {}
RethrowFn(RethrowFn), SavedExnVar(SavedExnVar),
FinallyExecutedFlag(FinallyExecutedFlag) {}

void Emit(CodeGenFunction &CGF, Flags flags) override {
// Only execute the finally block if it hasn't already run.
llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
CGF.EmitBlock(RunFinallyBB);
CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
// Enter a cleanup to call the end-catch function if one was provided.
if (EndCatchFn)
CGF.EHStack.pushCleanup<CallEndCatchForFinally>(NormalAndEHCleanup,
Expand Down Expand Up @@ -1429,6 +1439,7 @@ namespace {
// Now make sure we actually have an insertion point or the
// cleanup gods will hate us.
CGF.EnsureInsertPoint();
CGF.EmitBlock(SkipFinallyBB);
}
};
} // end anonymous namespace
Expand Down Expand Up @@ -1478,10 +1489,12 @@ void CodeGenFunction::FinallyInfo::enter(CodeGenFunction &CGF, const Stmt *body,
ForEHVar = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.for-eh");
CGF.Builder.CreateFlagStore(false, ForEHVar);

// Enter a normal cleanup which will perform the @finally block.
// Allocate a flag to ensure the finally block is only executed once.
llvm::Value *FinallyExecutedFlag = CGF.CreateTempAlloca(CGF.Builder.getInt1Ty(), "finally.executed");
CGF.Builder.CreateFlagStore(false, FinallyExecutedFlag);
CGF.EHStack.pushCleanup<PerformFinally>(NormalCleanup, body,
ForEHVar, endCatchFn,
rethrowFn, SavedExnVar);
rethrowFn, SavedExnVar, FinallyExecutedFlag);

// Enter a catch-all scope.
llvm::BasicBlock *catchBB = CGF.createBasicBlock("finally.catchall");
Expand Down Expand Up @@ -1724,10 +1737,18 @@ void CodeGenFunction::VolatilizeTryBlocks(
namespace {
struct PerformSEHFinally final : EHScopeStack::Cleanup {
llvm::Function *OutlinedFinally;
PerformSEHFinally(llvm::Function *OutlinedFinally)
: OutlinedFinally(OutlinedFinally) {}
llvm::Value *FinallyExecutedFlag;
PerformSEHFinally(llvm::Function *OutlinedFinally, llvm::Value *FinallyExecutedFlag)
: OutlinedFinally(OutlinedFinally), FinallyExecutedFlag(FinallyExecutedFlag) {}

void Emit(CodeGenFunction &CGF, Flags F) override {
// Only execute the finally block if it hasn't already run.
llvm::BasicBlock *RunFinallyBB = CGF.createBasicBlock("finally.run");
llvm::BasicBlock *SkipFinallyBB = CGF.createBasicBlock("finally.skip");
llvm::Value *AlreadyExecuted = CGF.Builder.CreateFlagLoad(FinallyExecutedFlag, "finally.executed");
CGF.Builder.CreateCondBr(AlreadyExecuted, SkipFinallyBB, RunFinallyBB);
CGF.EmitBlock(RunFinallyBB);
CGF.Builder.CreateFlagStore(true, FinallyExecutedFlag);
ASTContext &Context = CGF.getContext();
CodeGenModule &CGM = CGF.CGM;

Expand Down Expand Up @@ -1769,6 +1790,8 @@ struct PerformSEHFinally final : EHScopeStack::Cleanup {

auto Callee = CGCallee::forDirect(OutlinedFinally);
CGF.EmitCall(FnInfo, Callee, ReturnValueSlot(), Args);

CGF.EmitBlock(SkipFinallyBB);
}
};
} // end anonymous namespace
Expand Down Expand Up @@ -2164,7 +2187,10 @@ llvm::Value *CodeGenFunction::EmitSEHAbnormalTermination() {

void CodeGenFunction::pushSEHCleanup(CleanupKind Kind,
llvm::Function *FinallyFunc) {
EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc);
// Allocate a flag to ensure the finally block is only executed once.
llvm::Value *FinallyExecutedFlag = CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
Builder.CreateFlagStore(false, FinallyExecutedFlag);
EHStack.pushCleanup<PerformSEHFinally>(Kind, FinallyFunc, FinallyExecutedFlag);
}

void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
Expand All @@ -2175,8 +2201,11 @@ void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
llvm::Function *FinallyFunc =
HelperCGF.GenerateSEHFinallyFunction(*this, *Finally);

// Allocate a flag to ensure the finally block is only executed once.
llvm::Value *FinallyExecutedFlag = CreateTempAlloca(Builder.getInt1Ty(), "finally.executed");
Builder.CreateFlagStore(false, FinallyExecutedFlag);
// Push a cleanup for __finally blocks.
EHStack.pushCleanup<PerformSEHFinally>(NormalAndEHCleanup, FinallyFunc);
EHStack.pushCleanup<PerformSEHFinally>(NormalAndEHCleanup, FinallyFunc, FinallyExecutedFlag);
return;
}

Expand Down
34 changes: 34 additions & 0 deletions clang/test/CodeGen/seh-finally-double-execute.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -O0 -fms-extensions -fexceptions -fcxx-exceptions -o - %s | FileCheck %s

int freed = 0;
void myfree(int *p) {
++freed;
}

// CHECK-LABEL: define dso_local i32 @main(
int main() {
int x = 0;
int *p = &x;
__try {
return 0;
} __finally {
myfree(p);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code clang generates looks for this testcase looks fine? In fact, this testcase doesn't have an exception path at all. I'm not sure what you're trying to fix.

(Maybe see also https://reviews.llvm.org/D124642... which is vaguely related.)


// Check that a guard flag is allocated to prevent double execution
// CHECK: %finally.executed = alloca i1
// CHECK: store i1 false, ptr %finally.executed

// Check the main function has guard logic to prevent double execution
// CHECK: %finally.executed{{.*}} = load i1, ptr %finally.executed
// CHECK: br i1 %finally.executed{{.*}}, label %finally.skip, label %finally.run
// CHECK: finally.run:
// CHECK: store i1 true, ptr %finally.executed
// CHECK: call void @"?fin$0@0@main@@"
// CHECK: finally.skip:

// Check the finally helper function is called only once
// CHECK-LABEL: define internal void @"?fin$0@0@main@@"
// CHECK: call void @myfree
// CHECK-NOT: call void @myfree
Loading