From 3a237aa31ca3add00459a3c029479e3d34b645b0 Mon Sep 17 00:00:00 2001 From: John McCall Date: Wed, 9 Nov 2011 03:17:26 +0000 Subject: [PATCH] Emit the copy and dipose helpers for ARC __strong block-typed __block variables using objc_retainBlock and objc_dispose. Previously we were using _Block_object_assign and _Block_object_destroy with BLOCK_BYREF_CALLER, which causes the runtime to completely ignore the retain and release. In most cases this doesn't cause catastrophe because the retain/release are balanced and because the block in the variable was copied upon assignment there. However, the stack copy of the variable will be released when it goes out of scope, which is a problem if that value was released due to an assignment to the heap copy. Similarly, a leak can occur if the variable is assigned after the copy to the heap. llvm-svn: 144162 --- clang/lib/CodeGen/CGBlocks.cpp | 58 ++++++++++++++++++++++++----- clang/test/CodeGenObjC/arc-blocks.m | 29 ++++++++++++++- 2 files changed, 76 insertions(+), 11 deletions(-) diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp index 076e20c5fbba..ce9e30ceba1c 100644 --- a/clang/lib/CodeGen/CGBlocks.cpp +++ b/clang/lib/CodeGen/CGBlocks.cpp @@ -1342,15 +1342,23 @@ public: // Do a "move" by copying the value and then zeroing out the old // variable. - llvm::Value *value = CGF.Builder.CreateLoad(srcField); + llvm::LoadInst *value = CGF.Builder.CreateLoad(srcField); + value->setAlignment(Alignment.getQuantity()); + llvm::Value *null = llvm::ConstantPointerNull::get(cast(value->getType())); - CGF.Builder.CreateStore(value, destField); - CGF.Builder.CreateStore(null, srcField); + + llvm::StoreInst *store = CGF.Builder.CreateStore(value, destField); + store->setAlignment(Alignment.getQuantity()); + + store = CGF.Builder.CreateStore(null, srcField); + store->setAlignment(Alignment.getQuantity()); } void emitDispose(CodeGenFunction &CGF, llvm::Value *field) { - llvm::Value *value = CGF.Builder.CreateLoad(field); + llvm::LoadInst *value = CGF.Builder.CreateLoad(field); + value->setAlignment(Alignment.getQuantity()); + CGF.EmitARCRelease(value, /*precise*/ false); } @@ -1360,6 +1368,39 @@ public: } }; +/// Emits the copy/dispose helpers for an ARC __block __strong +/// variable that's of block-pointer type. +class ARCStrongBlockByrefHelpers : public CodeGenModule::ByrefHelpers { +public: + ARCStrongBlockByrefHelpers(CharUnits alignment) : ByrefHelpers(alignment) {} + + void emitCopy(CodeGenFunction &CGF, llvm::Value *destField, + llvm::Value *srcField) { + // Do the copy with objc_retainBlock; that's all that + // _Block_object_assign would do anyway, and we'd have to pass the + // right arguments to make sure it doesn't get no-op'ed. + llvm::LoadInst *oldValue = CGF.Builder.CreateLoad(srcField); + oldValue->setAlignment(Alignment.getQuantity()); + + llvm::Value *copy = CGF.EmitARCRetainBlock(oldValue, /*mandatory*/ true); + + llvm::StoreInst *store = CGF.Builder.CreateStore(copy, destField); + store->setAlignment(Alignment.getQuantity()); + } + + void emitDispose(CodeGenFunction &CGF, llvm::Value *field) { + llvm::LoadInst *value = CGF.Builder.CreateLoad(field); + value->setAlignment(Alignment.getQuantity()); + + CGF.EmitARCRelease(value, /*precise*/ false); + } + + void profileImpl(llvm::FoldingSetNodeID &id) const { + // 2 is distinguishable from all pointers and byref flags + id.AddInteger(2); + } +}; + /// Emits the copy/dispose helpers for a __block variable with a /// nontrivial copy constructor or destructor. class CXXByrefHelpers : public CodeGenModule::ByrefHelpers { @@ -1586,13 +1627,10 @@ CodeGenFunction::buildByrefHelpers(llvm::StructType &byrefType, // ARC __strong __block variables need to be retained. case Qualifiers::OCL_Strong: - // Block-pointers need to be _Block_copy'ed, so we let the - // runtime be in charge. But we can't use the code below - // because we don't want to set BYREF_CALLER, which will - // just make the runtime ignore us. + // Block pointers need to be copied, and there's no direct + // transfer possible. if (type->isBlockPointerType()) { - BlockFieldFlags flags = BLOCK_FIELD_IS_BLOCK; - ObjectByrefHelpers byrefInfo(emission.Alignment, flags); + ARCStrongBlockByrefHelpers byrefInfo(emission.Alignment); return ::buildByrefHelpers(CGM, byrefType, byrefInfo); // Otherwise, we transfer ownership of the retain from the stack diff --git a/clang/test/CodeGenObjC/arc-blocks.m b/clang/test/CodeGenObjC/arc-blocks.m index 50fdd0a12c77..38675a439334 100644 --- a/clang/test/CodeGenObjC/arc-blocks.m +++ b/clang/test/CodeGenObjC/arc-blocks.m @@ -334,6 +334,34 @@ void test10a(void) { // CHECK-NEXT: ret void } +// : do this copy and dispose with +// objc_retainBlock/release instead of _Block_object_assign/destroy. +// We can also use _Block_object_assign/destroy with +// BLOCK_FIELD_IS_BLOCK as long as we don't pass BLOCK_BYREF_CALLER. + +// CHECK: define internal void @__Block_byref_object_copy +// CHECK: [[D0:%.*]] = load i8** {{%.*}} +// CHECK-NEXT: [[D1:%.*]] = bitcast i8* [[D0]] to [[BYREF_T]]* +// CHECK-NEXT: [[D2:%.*]] = getelementptr inbounds [[BYREF_T]]* [[D1]], i32 0, i32 6 +// CHECK-NEXT: [[S0:%.*]] = load i8** {{%.*}} +// CHECK-NEXT: [[S1:%.*]] = bitcast i8* [[S0]] to [[BYREF_T]]* +// CHECK-NEXT: [[S2:%.*]] = getelementptr inbounds [[BYREF_T]]* [[S1]], i32 0, i32 6 +// CHECK-NEXT: [[T0:%.*]] = load void ()** [[S2]], align 8 +// CHECK-NEXT: [[T1:%.*]] = bitcast void ()* [[T0]] to i8* +// CHECK-NEXT: [[T2:%.*]] = call i8* @objc_retainBlock(i8* [[T1]]) +// CHECK-NEXT: [[T3:%.*]] = bitcast i8* [[T2]] to void ()* +// CHECK-NEXT: store void ()* [[T3]], void ()** [[D2]], align 8 +// CHECK-NEXT: ret void + +// CHECK: define internal void @__Block_byref_object_dispose +// CHECK: [[T0:%.*]] = load i8** {{%.*}} +// CHECK-NEXT: [[T1:%.*]] = bitcast i8* [[T0]] to [[BYREF_T]]* +// CHECK-NEXT: [[T2:%.*]] = getelementptr inbounds [[BYREF_T]]* [[T1]], i32 0, i32 6 +// CHECK-NEXT: [[T3:%.*]] = load void ()** [[T2]], align 8 +// CHECK-NEXT: [[T4:%.*]] = bitcast void ()* [[T3]] to i8* +// CHECK-NEXT: call void @objc_release(i8* [[T4]]) +// CHECK-NEXT: ret void + // Test that we correctly assign to __block variables when the // assignment captures the variable. void test10b(void) { @@ -427,4 +455,3 @@ void test11b(void) { // CHECK: define internal void @"\01-[Test12 setNblock:]"( // CHECK: call void @objc_setProperty(i8* {{%.*}}, i8* {{%.*}}, i64 {{%.*}}, i8* {{%.*}}, i1 zeroext false, i1 zeroext true) @end - -- GitLab