diff --git a/clang/lib/CodeGen/CGException.cpp b/clang/lib/CodeGen/CGException.cpp index ad138b9876e8c..f3cbb77d810a7 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(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(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(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(Kind, FinallyFunc, FinallyExecutedFlag); } void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) { @@ -2176,7 +2202,7 @@ void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) { HelperCGF.GenerateSEHFinallyFunction(*this, *Finally); // Push a cleanup for __finally blocks. - EHStack.pushCleanup(NormalAndEHCleanup, FinallyFunc); + pushSEHCleanup(NormalAndEHCleanup, FinallyFunc); return; } @@ -2209,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 new file mode 100644 index 0000000000000..5924993ca78a5 --- /dev/null +++ b/clang/test/CodeGen/seh-finally-double-execute.c @@ -0,0 +1,114 @@ +// 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* 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; +} + +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 { + // 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 { + // 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: 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; +}