ahatanak created this revision.
ahatanak added a reviewer: rjmccall.
Herald added a subscriber: dexonsmith.
Block copy/dispose helper functions currently aren’t exception-safe. When an
exception is thrown in a copy function, the function exits without destroying
or releasing C++ objects, ObjC objects, and `__block` objects that have already
been copied. Similarly, dispose functions don't release ObjC objects and
`__block objects` that are yet to be released when an exception is thrown.
Repository:
rC Clang
https://reviews.llvm.org/D49718
Files:
lib/CodeGen/CGBlocks.cpp
test/CodeGenObjCXX/arc-blocks.mm
Index: test/CodeGenObjCXX/arc-blocks.mm
===================================================================
--- test/CodeGenObjCXX/arc-blocks.mm
+++ test/CodeGenObjCXX/arc-blocks.mm
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 -std=gnu++98 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-runtime-has-weak -fblocks -fobjc-arc -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=gnu++98 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-runtime-has-weak -fblocks -fobjc-arc -fexceptions -fobjc-arc-exceptions -o - %s | FileCheck -check-prefix CHECK %s
+// RUN: %clang_cc1 -std=gnu++98 -triple x86_64-apple-darwin10 -emit-llvm -fobjc-runtime-has-weak -fblocks -fobjc-arc -fexceptions -fobjc-arc-exceptions -O1 -o - %s | FileCheck -check-prefix CHECK-O1 %s
// CHECK: [[A:.*]] = type { i64, [10 x i8*] }
+// CHECK: %[[STRUCT_TEST1_S0:.*]] = type { i32 }
+// CHECK: %[[STRUCT_BLOCK_DESCRIPTOR:.*]] = type { i64, i64 }
// CHECK: [[LAYOUT0:@.*]] = private unnamed_addr constant [3 x i8] c" 9\00"
@@ -47,3 +50,133 @@
// CHECK-NEXT: call void @_ZN5test01AD1Ev([[A]]* [[T1]])
// CHECK-NEXT: ret void
}
+
+namespace test1 {
+
+// Check that copy/dispose helper functions are exception safe.
+
+// CHECK-LABEL: define internal void @__copy_helper_block_(
+// CHECK: %[[BLOCK_SOURCE:.*]] = bitcast i8* %{{.*}} to <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>*
+// CHECK: %[[BLOCK_DEST:.*]] = bitcast i8* %{{.*}} to <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>*
+
+// CHECK: %[[V4:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_SOURCE]], i32 0, i32 6
+// CHECK: %[[V5:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_DEST]], i32 0, i32 6
+// CHECK: %[[BLOCKCOPY_SRC:.*]] = load i8*, i8** %[[V4]], align 8
+// CHECK: %[[V6:.*]] = bitcast i8** %[[V5]] to i8*
+// CHECK: call void @_Block_object_assign(i8* %[[V6]], i8* %[[BLOCKCOPY_SRC]], i32 8)
+
+// CHECK: %[[V7:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_SOURCE]], i32 0, i32 7
+// CHECK: %[[V8:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_DEST]], i32 0, i32 7
+// CHECK: call void @objc_copyWeak(i8** %[[V8]], i8** %[[V7]])
+
+// CHECK: %[[V9:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_SOURCE]], i32 0, i32 5
+// CHECK: %[[V10:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_DEST]], i32 0, i32 5
+// CHECK: %[[BLOCKCOPY_SRC2:.*]] = load i8*, i8** %[[V9]], align 8
+// CHECK: store i8* null, i8** %[[V10]], align 8
+// CHECK: call void @objc_storeStrong(i8** %[[V10]], i8* %[[BLOCKCOPY_SRC2]])
+
+// CHECK: %[[V11:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_SOURCE]], i32 0, i32 8
+// CHECK: %[[V12:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_DEST]], i32 0, i32 8
+// CHECK: invoke void @_ZN5test12S0C1ERKS0_(%[[STRUCT_TEST1_S0]]* %[[V12]], %[[STRUCT_TEST1_S0]]* dereferenceable(4) %[[V11]])
+// CHECK: to label %[[INVOKE_CONT:.*]] unwind label %[[LPAD:.*]]
+
+// CHECK: [[INVOKE_CONT]]:
+// CHECK: %[[V13:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_SOURCE]], i32 0, i32 9
+// CHECK: %[[V14:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK_DEST]], i32 0, i32 9
+// CHECK: invoke void @_ZN5test12S0C1ERKS0_(%[[STRUCT_TEST1_S0]]* %[[V14]], %[[STRUCT_TEST1_S0]]* dereferenceable(4) %[[V13]])
+// CHECK: to label %[[INVOKE_CONT4:.*]] unwind label %[[LPAD3:.*]]
+
+// CHECK: [[INVOKE_CONT4]]:
+// CHECK: ret void
+
+// CHECK: [[LPAD]]:
+// CHECK: br label %[[EHCLEANUP:.*]]
+
+// CHECK: [[LPAD3]]:
+// CHECK: invoke void @_ZN5test12S0D1Ev(%[[STRUCT_TEST1_S0]]* %[[V12]])
+// CHECK: to label %[[INVOKE_CONT5:.*]] unwind label %[[TERMINATE_LPAD:.*]]
+
+// CHECK: [[INVOKE_CONT5]]:
+// CHECK: br label %[[EHCLEANUP]]
+
+// CHECK: [[EHCLEANUP]]:
+// CHECK: call void @objc_storeStrong(i8** %[[V10]], i8* null)
+// CHECK: call void @objc_destroyWeak(i8** %[[V8]])
+// CHECK: %[[V21:.*]] = load i8*, i8** %[[V5]], align 8
+// CHECK: call void @_Block_object_dispose(i8* %[[V21]], i32 8)
+// CHECK: br label %[[EH_RESUME:.*]]
+
+// CHECK: [[EH_RESUME]]:
+// CHECK: resume { i8*, i32 }
+
+// CHECK: [[TERMINATE_LPAD]]:
+// CHECK: call void @__clang_call_terminate(
+
+// CHECK-O1-LABEL: define internal void @__copy_helper_block_(
+// CHECK-O1: tail call void @objc_release({{.*}}) {{.*}} !clang.imprecise_release
+
+// CHECK: define internal void @__destroy_helper_block_(
+// CHECK: %[[BLOCK:.*]] = bitcast i8* %{{.*}} to <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>*
+// CHECK: %[[V2:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK]], i32 0, i32 6
+// CHECK: %[[V3:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK]], i32 0, i32 7
+// CHECK: %[[V4:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK]], i32 0, i32 5
+// CHECK: %[[V5:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK]], i32 0, i32 8
+// CHECK: %[[V6:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8*, %[[STRUCT_TEST1_S0]], %[[STRUCT_TEST1_S0]] }>* %[[BLOCK]], i32 0, i32 9
+// CHECK: invoke void @_ZN5test12S0D1Ev(%[[STRUCT_TEST1_S0]]* %[[V6]])
+// CHECK: to label %[[INVOKE_CONT:.*]] unwind label %[[LPAD:.*]]
+
+// CHECK: [[INVOKE_CONT]]:
+// CHECK: invoke void @_ZN5test12S0D1Ev(%[[STRUCT_TEST1_S0]]* %[[V5]])
+// CHECK: to label %[[INVOKE_CONT2:.*]] unwind label %[[LPAD1:.*]]
+
+// CHECK: [[INVOKE_CONT2]]:
+// CHECK: call void @objc_storeStrong(i8** %[[V4]], i8* null)
+// CHECK: call void @objc_destroyWeak(i8** %[[V3]])
+// CHECK: %[[V7:.*]] = load i8*, i8** %[[V2]], align 8
+// CHECK: call void @_Block_object_dispose(i8* %[[V7]], i32 8)
+// CHECK: ret void
+
+// CHECK: [[LPAD]]:
+// CHECK: invoke void @_ZN5test12S0D1Ev(%[[STRUCT_TEST1_S0]]* %[[V5]])
+// CHECK: to label %[[INVOKE_CONT3:.*]] unwind label %[[TERMINATE_LPAD]]
+
+// CHECK: [[LPAD1]]
+// CHECK: br label %[[EHCLEANUP:.*]]
+
+// CHECK: [[INVOKE_CONT3]]:
+// CHECK: br label %[[EHCLEANUP]]
+
+// CHECK: [[EHCLEANUP]]:
+// CHECK: call void @objc_storeStrong(i8** %[[V4]], i8* null)
+// CHECK: call void @objc_destroyWeak(i8** %[[V3]])
+// CHECK: %[[V14:.*]] = load i8*, i8** %[[V2]], align 8
+// CHECK: call void @_Block_object_dispose(i8* %[[V14]], i32 8)
+// CHECK: br label %[[EH_RESUME]]
+
+// CHECK: [[EH_RESUME]]:
+// CHECK: resume { i8*, i32 }
+
+// CHECK: [[TERMINATE_LPAD]]:
+// CHECK: call void @__clang_call_terminate(
+
+// CHECK-O1-LABEL: define internal void @__destroy_helper_block_(
+// CHECK-O1: tail call void @objc_release({{.*}}) {{.*}} !clang.imprecise_release
+// CHECK-O1: tail call void @objc_release({{.*}}) {{.*}} !clang.imprecise_release
+
+struct S0 {
+ S0();
+ S0(const S0 &);
+ ~S0();
+ int f0;
+};
+
+id getObj();
+
+void foo1() {
+ __block id t0 = getObj();
+ __weak id t1 = getObj();
+ id t2 = getObj();
+ S0 t3, t4;
+ ^{ (void)t0; (void)t1; (void)t2; (void)t3; (void)t4; };
+}
+}
Index: lib/CodeGen/CGBlocks.cpp
===================================================================
--- lib/CodeGen/CGBlocks.cpp
+++ lib/CodeGen/CGBlocks.cpp
@@ -1585,6 +1585,29 @@
}
}
+namespace {
+ // Cleanup for releasing __block variables in copy/destroy helper functions.
+ struct BlockObjectRelease final : EHScopeStack::Cleanup {
+ BlockObjectRelease(Address field, BlockFieldFlags flags)
+ : field(field), blockFieldFlags(flags) {}
+
+ Address field;
+ BlockFieldFlags blockFieldFlags;
+
+ void Emit(CodeGenFunction &CGF, Flags flags) override {
+ llvm::Value *value = CGF.Builder.CreateLoad(field);
+ value = CGF.Builder.CreateBitCast(value, CGF.VoidPtrTy);
+ CGF.BuildBlockRelease(value, blockFieldFlags);
+ }
+ };
+}
+
+static void pushBlockObjectRelease(CleanupKind cleanupKind, Address field,
+ BlockFieldFlags flags,
+ CodeGenFunction &CGF) {
+ CGF.EHStack.pushCleanup<BlockObjectRelease>(cleanupKind, field, flags);
+}
+
/// Generate the copy-helper function for a block closure object:
/// static void block_copy_helper(block_t *dst, block_t *src);
/// The runtime will have previously initialized 'dst' by doing a
@@ -1648,6 +1671,7 @@
for (const auto &CopiedCapture : CopiedCaptures) {
const BlockDecl::Capture &CI = CopiedCapture.CI;
const CGBlockInfo::Capture &capture = CopiedCapture.Capture;
+ QualType captureType = CI.getVariable()->getType();
BlockFieldFlags flags = CopiedCapture.Flags;
unsigned index = capture.getIndex();
@@ -1685,9 +1709,11 @@
} else {
EmitARCRetainNonBlock(srcValue);
- // We don't need this anymore, so kill it. It's not quite
- // worth the annoyance to avoid creating it in the first place.
- cast<llvm::Instruction>(dstField.getPointer())->eraseFromParent();
+ // Unless EH cleanup is required, we don't need this anymore, so kill
+ // it. It's not quite worth the annoyance to avoid creating it in the
+ // first place.
+ if (!needsEHCleanup(captureType.isDestructedType()))
+ cast<llvm::Instruction>(dstField.getPointer())->eraseFromParent();
}
} else {
assert(CopiedCapture.Kind == BlockCaptureEntityKind::BlockObject);
@@ -1715,6 +1741,31 @@
}
}
}
+
+ // Ensure that we destroy the copied object if an exception is thrown later
+ // in the helper function.
+ switch (CopiedCapture.Kind) {
+ case BlockCaptureEntityKind::CXXRecord:
+ case BlockCaptureEntityKind::ARCWeak:
+ case BlockCaptureEntityKind::NonTrivialCStruct:
+ if (captureType.isDestructedType() &&
+ needsEHCleanup(captureType.isDestructedType()))
+ pushEHDestroy(captureType.isDestructedType(), dstField, captureType);
+ break;
+ case BlockCaptureEntityKind::ARCStrong: {
+ if (needsEHCleanup(captureType.isDestructedType()))
+ pushDestroy(EHCleanup, dstField, captureType,
+ CodeGenFunction::destroyARCStrongImprecise, true);
+ break;
+ }
+ case BlockCaptureEntityKind::BlockObject: {
+ if (getLangOpts().Exceptions)
+ pushBlockObjectRelease(EHCleanup, dstField, flags, *this);
+ break;
+ }
+ case BlockCaptureEntityKind::None:
+ llvm_unreachable("unexpected BlockCaptureEntityKind");
+ }
}
FinishFunction();
@@ -1826,39 +1877,32 @@
const BlockDecl::Capture &CI = DestroyedCapture.CI;
const CGBlockInfo::Capture &capture = DestroyedCapture.Capture;
BlockFieldFlags flags = DestroyedCapture.Flags;
+ QualType captureType = CI.getVariable()->getType();
Address srcField =
Builder.CreateStructGEP(src, capture.getIndex(), capture.getOffset());
- // If the captured record has a destructor then call it.
- if (DestroyedCapture.Kind == BlockCaptureEntityKind::CXXRecord) {
- const auto *Dtor =
- CI.getVariable()->getType()->getAsCXXRecordDecl()->getDestructor();
- PushDestructorCleanup(Dtor, srcField);
-
- // If this is a __weak capture, emit the release directly.
- } else if (DestroyedCapture.Kind == BlockCaptureEntityKind::ARCWeak) {
- EmitARCDestroyWeak(srcField);
-
- // Destroy strong objects with a call if requested.
- } else if (DestroyedCapture.Kind == BlockCaptureEntityKind::ARCStrong) {
- EmitARCDestroyStrong(srcField, ARCImpreciseLifetime);
-
- // If this is a C struct that requires non-trivial destruction, emit a call
- // to its destructor.
- } else if (DestroyedCapture.Kind ==
- BlockCaptureEntityKind::NonTrivialCStruct) {
- QualType varType = CI.getVariable()->getType();
- pushDestroy(varType.isDestructedType(), srcField, varType);
-
- // Otherwise we call _Block_object_dispose. It wouldn't be too
- // hard to just emit this as a cleanup if we wanted to make sure
- // that things were done in reverse.
- } else {
- assert(DestroyedCapture.Kind == BlockCaptureEntityKind::BlockObject);
- llvm::Value *value = Builder.CreateLoad(srcField);
- value = Builder.CreateBitCast(value, VoidPtrTy);
- BuildBlockRelease(value, flags);
+ switch (DestroyedCapture.Kind) {
+ case BlockCaptureEntityKind::CXXRecord:
+ case BlockCaptureEntityKind::ARCWeak:
+ case BlockCaptureEntityKind::NonTrivialCStruct:
+ pushDestroy(captureType.isDestructedType(), srcField, captureType);
+ break;
+ case BlockCaptureEntityKind::ARCStrong: {
+ CleanupKind cleanupKind = getCleanupKind(captureType.isDestructedType());
+ pushDestroy(cleanupKind, srcField, captureType,
+ CodeGenFunction::destroyARCStrongImprecise,
+ cleanupKind & EHCleanup);
+ break;
+ }
+ case BlockCaptureEntityKind::BlockObject: {
+ CleanupKind kind =
+ getLangOpts().Exceptions ? NormalAndEHCleanup : NormalCleanup;
+ pushBlockObjectRelease(kind, srcField, flags, *this);
+ break;
+ }
+ case BlockCaptureEntityKind::None:
+ llvm_unreachable("unexpected BlockCaptureEntityKind");
}
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
