[Clang] Fixed double finally block execution

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) {
@@ -2176,7 +2202,7 @@ void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
         HelperCGF.GenerateSEHFinallyFunction(*this, *Finally);
 
     // Push a cleanup for __finally blocks.
-    EHStack.pushCleanup<PerformSEHFinally>(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;
   }
 

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;
+}