https://github.com/yasster updated
https://github.com/llvm/llvm-project/pull/146796
>From 2ccf38469c28392597199e98515dd5a21cfc7c84 Mon Sep 17 00:00:00 2001
From: Yassine Missoum <mmiss...@microsoft.com>
Date: Tue, 1 Jul 2025 15:25:21 -0700
Subject: [PATCH 1/2] Fixed double finally block execution
---
clang/lib/CodeGen/CGException.cpp | 45 +++++++++++++++----
.../test/CodeGen/seh-finally-double-execute.c | 34 ++++++++++++++
2 files changed, 71 insertions(+), 8 deletions(-)
create mode 100644 clang/test/CodeGen/seh-finally-double-execute.c
diff --git a/clang/lib/CodeGen/CGException.cpp
b/clang/lib/CodeGen/CGException.cpp
index ad138b9876e8c..ab4086716cc1c 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -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,
@@ -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
@@ -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");
@@ -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;
@@ -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
@@ -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) {
@@ -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;
}
diff --git a/clang/test/CodeGen/seh-finally-double-execute.c
b/clang/test/CodeGen/seh-finally-double-execute.c
new file mode 100644
index 0000000000000..0f2d417e0f4fb
--- /dev/null
+++ b/clang/test/CodeGen/seh-finally-double-execute.c
@@ -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);
+ }
+}
+
+// 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
>From 4c99b3f9dbdfa408169c4167138e64debae40233 Mon Sep 17 00:00:00 2001
From: Yassine Missoum <mmiss...@microsoft.com>
Date: Mon, 7 Jul 2025 10:58:17 -0700
Subject: [PATCH 2/2] Updated the test and the code to fix double finally issue
---
clang/lib/CodeGen/CGException.cpp | 13 +-
.../test/CodeGen/seh-finally-double-execute.c | 130 ++++++++++++++----
2 files changed, 113 insertions(+), 30 deletions(-)
diff --git a/clang/lib/CodeGen/CGException.cpp
b/clang/lib/CodeGen/CGException.cpp
index ab4086716cc1c..f3cbb77d810a7 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -2201,11 +2201,8 @@ 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,
FinallyExecutedFlag);
+ pushSEHCleanup(NormalAndEHCleanup, FinallyFunc);
return;
}
@@ -2238,7 +2235,13 @@ void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt
&S) {
void CodeGenFunction::ExitSEHTryStmt(const SEHTryStmt &S) {
// Just pop the cleanup if it's a __finally block.
if (S.getFinallyHandler()) {
- PopCleanupBlock();
+ // In most cases, the cleanup is active, but if the __try contains a
+ // noreturn call, the block will be terminated and the cleanup will be
+ // inactive.
+ if (HaveInsertPoint())
+ PopCleanupBlock();
+ else
+ EHStack.popCleanup();
return;
}
diff --git a/clang/test/CodeGen/seh-finally-double-execute.c
b/clang/test/CodeGen/seh-finally-double-execute.c
index 0f2d417e0f4fb..5924993ca78a5 100644
--- a/clang/test/CodeGen/seh-finally-double-execute.c
+++ b/clang/test/CodeGen/seh-finally-double-execute.c
@@ -1,34 +1,114 @@
-// RUN: %clang_cc1 -triple x86_64-windows-msvc -emit-llvm -O0 -fms-extensions
-fexceptions -fcxx-exceptions -o - %s | FileCheck %s
+// RUN: %clang_cc1 -x c -triple x86_64-windows-msvc -emit-llvm -O0
-fms-extensions -fexceptions -o - %s | FileCheck %s
+// Global state to track resource cleanup
int freed = 0;
-void myfree(int *p) {
- ++freed;
+void* allocated_buffer = 0;
+
+// External functions that prevent optimization
+extern void external_operation(void);
+extern int external_condition(void);
+extern void external_cleanup(void*);
+
+// Declare SEH exception functions
+void RaiseException(unsigned long code, unsigned long flags, unsigned long
argc, void* argv);
+
+// Simulate complex resource allocation/cleanup
+void* allocate_buffer(int size) {
+ // Simulate allocation that could fail
+ if (external_condition()) {
+ allocated_buffer = (void*)0x12345678; // Mock allocation
+ return allocated_buffer;
+ }
+ return 0;
}
-// CHECK-LABEL: define dso_local i32 @main(
-int main() {
- int x = 0;
- int *p = &x;
+void free_buffer(void* buffer) {
+ if (buffer && freed == 0) {
+ freed = 1;
+ allocated_buffer = 0;
+ external_cleanup(buffer); // External cleanup prevents inlining
+ }
+}
+
+
+int complex_operation_with_finally(int operation_type) {
+ void* buffer = 0;
+ int result = 0;
+
__try {
- return 0;
+ // Multiple operations that could throw exceptions
+ buffer = allocate_buffer(1024);
+ if (!buffer) {
+ result = -1;
+ __leave; // Early exit - finally should still run
+ }
+
+ // Simulate complex operations that could throw
+ external_operation(); // Could throw
+
+ if (operation_type == 1) {
+ external_operation(); // Another potential throw point
+ }
+
+ result = 0; // Success
} __finally {
- myfree(p);
+ // Critical cleanup that must run exactly once
+ if (buffer) {
+ free_buffer(buffer);
+ }
+ }
+
+ // Exception raised after finally block has already executed
+ // This is the pattern that causes double execution in resource cleanup
+ if (operation_type == 2) {
+ RaiseException(0xC0000005, 0, 0, 0);
}
+
+ return result;
}
-// 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
+// CHECK: define dso_local i32 @complex_operation_with_finally(i32 noundef
%operation_type)
+// CHECK: %finally.executed = alloca i1, align 1
+// CHECK: store i1 false, ptr %finally.executed, align 1
+
+// Normal path: check if finally already ran.
+// CHECK-LABEL: __try.__leave:
+// CHECK: %[[finally_executed:.+]] = load i1, ptr %finally.executed, align 1
+// CHECK: br i1 %[[finally_executed]], label %finally.skip, label
%finally.run
+
+// Normal path: run finally and set flag.
+// CHECK-LABEL: finally.run:
+// CHECK: store i1 true, ptr %finally.executed, align 1
+// CHECK: call void @"?fin$0@0@complex_operation_with_finally@@"(i8 noundef
+// CHECK: br label %finally.skip
+
+// Normal path: skip finally.
+// CHECK-LABEL: finally.skip:
+// CHECK: %[[cmp:.+]] = icmp eq i32 {{.+}}, 2
+// CHECK: br i1 %[[cmp]], label %if.then10, label %if.end11
+
+// Exception path: check if finally already ran.
+// CHECK-LABEL: ehcleanup:
+// CHECK: %[[finally_executed_eh:.+]] = load i1, ptr %finally.executed,
align 1
+// CHECK: br i1 %[[finally_executed_eh]], label %finally.skip8, label
%finally.run7
+
+// Exception path: run finally and set flag.
+// CHECK-LABEL: finally.run7:
+// CHECK: store i1 true, ptr %finally.executed, align 1
+// CHECK: call void @"?fin$0@0@complex_operation_with_finally@@"(i8 noundef 1
+// CHECK: br label %finally.skip8
+
+// Exception path: skip finally.
+// CHECK-LABEL: finally.skip8:
+// CHECK: cleanupret from {{.+}} unwind to caller
+
+// CHECK: define internal void @"?fin$0@0@complex_operation_with_finally@@"(i8
noundef %abnormal_termination, ptr noundef %frame_pointer)
+// CHECK: call void @free_buffer(ptr noundef
+
+// CHECK-LABEL: @main
+int main() {
+ // This tests that the finally is not executed twice when an exception
+ // is raised after the finally has already run.
+ int result = complex_operation_with_finally(2);
+ return result;
+}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
