From 82c458ea76bf8f0981e3d1b5070c0b0e5878d784 Mon Sep 17 00:00:00 2001 From: Fariborz Jahanian Date: Tue, 27 Nov 2012 23:02:53 +0000 Subject: [PATCH] objective-C arc: load of a __weak object happens via call to objc_loadWeak. This retains and autorelease the weakly-refereced object. This hidden autorelease sometimes makes __weak variable alive even after the weak reference is erased, because the object is still referenced by an autorelease pool. This patch overcomes this behavior by loading a weak object via call to objc_loadWeakRetained(), followng it by objc_release at appropriate place, thereby removing the hidden autorelease. // rdar://10849570 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@168740 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGCall.cpp | 16 +++- lib/CodeGen/CGExpr.cpp | 7 +- lib/Sema/SemaExpr.cpp | 6 ++ lib/Sema/SemaInit.cpp | 26 +++++-- test/CodeGenObjC/arc-blocks.m | 5 +- test/CodeGenObjC/arc-foreach.m | 3 +- .../arc-loadweakretained-release.m | 77 +++++++++++++++++++ test/CodeGenObjC/arc.m | 7 +- test/CodeGenObjCXX/arc.mm | 7 +- 9 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 test/CodeGenObjC/arc-loadweakretained-release.m diff --git a/lib/CodeGen/CGCall.cpp b/lib/CodeGen/CGCall.cpp index 2d1d152894..c470918dde 100644 --- a/lib/CodeGen/CGCall.cpp +++ b/lib/CodeGen/CGCall.cpp @@ -1740,7 +1740,12 @@ static void emitWritebackArg(CodeGenFunction &CGF, CallArgList &args, // Create the temporary. llvm::Value *temp = CGF.CreateTempAlloca(destType->getElementType(), "icr.temp"); - + // Loading an l-value can introduce a cleanup if the l-value is __weak, + // and that cleanup will be conditional if we can't prove that the l-value + // isn't null, so we need to register a dominating point so that the cleanups + // system will make valid IR. + CodeGenFunction::ConditionalEvaluation condEval(CGF); + // Zero-initialize it if we're not doing a copy-initialization. bool shouldCopy = CRE->shouldCopy(); if (!shouldCopy) { @@ -1749,7 +1754,7 @@ static void emitWritebackArg(CodeGenFunction &CGF, CallArgList &args, cast(destType->getElementType())); CGF.Builder.CreateStore(null, temp); } - + llvm::BasicBlock *contBB = 0; // If the address is *not* known to be non-null, we need to switch. @@ -1772,6 +1777,7 @@ static void emitWritebackArg(CodeGenFunction &CGF, CallArgList &args, llvm::BasicBlock *copyBB = CGF.createBasicBlock("icr.copy"); CGF.Builder.CreateCondBr(isNull, contBB, copyBB); CGF.EmitBlock(copyBB); + condEval.begin(CGF); } } @@ -1788,10 +1794,12 @@ static void emitWritebackArg(CodeGenFunction &CGF, CallArgList &args, // Use an ordinary store, not a store-to-lvalue. CGF.Builder.CreateStore(src, temp); } - + // Finish the control flow if we needed it. - if (shouldCopy && !provablyNonNull) + if (shouldCopy && !provablyNonNull) { CGF.EmitBlock(contBB); + condEval.end(CGF); + } args.addWriteback(srcAddr, srcAddrType, temp); args.add(RValue::get(finalArgument), CRE->getType()); diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp index cb274fc406..e709e729b3 100644 --- a/lib/CodeGen/CGExpr.cpp +++ b/lib/CodeGen/CGExpr.cpp @@ -1119,8 +1119,11 @@ RValue CodeGenFunction::EmitLoadOfLValue(LValue LV) { return RValue::get(CGM.getObjCRuntime().EmitObjCWeakRead(*this, AddrWeakObj)); } - if (LV.getQuals().getObjCLifetime() == Qualifiers::OCL_Weak) - return RValue::get(EmitARCLoadWeak(LV.getAddress())); + if (LV.getQuals().getObjCLifetime() == Qualifiers::OCL_Weak) { + llvm::Value *Object = EmitARCLoadWeakRetained(LV.getAddress()); + Object = EmitObjCConsumeObject(LV.getType(), Object); + return RValue::get(Object); + } if (LV.isSimple()) { assert(!LV.getType()->isFunctionType()); diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 840f04f0da..66beb34128 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -504,6 +504,12 @@ ExprResult Sema::DefaultLvalueConversion(Expr *E) { T = T.getUnqualifiedType(); UpdateMarkingForLValueToRValue(E); + + // Loading a __weak object implicitly retains the value, so we need a cleanup to + // balance that. + if (getLangOpts().ObjCAutoRefCount && + E->getType().getObjCLifetime() == Qualifiers::OCL_Weak) + ExprNeedsCleanups = true; ExprResult Res = Owned(ImplicitCastExpr::Create(Context, T, CK_LValueToRValue, E, 0, VK_RValue)); diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp index 7ed3942332..5b7fb4656d 100644 --- a/lib/Sema/SemaInit.cpp +++ b/lib/Sema/SemaInit.cpp @@ -3846,14 +3846,15 @@ enum InvalidICRKind { IIK_okay, IIK_nonlocal, IIK_nonscalar }; /// Determines whether this expression is an acceptable ICR source. static InvalidICRKind isInvalidICRSource(ASTContext &C, Expr *e, - bool isAddressOf) { + bool isAddressOf, bool &isWeakAccess) { // Skip parens. e = e->IgnoreParens(); // Skip address-of nodes. if (UnaryOperator *op = dyn_cast(e)) { if (op->getOpcode() == UO_AddrOf) - return isInvalidICRSource(C, op->getSubExpr(), /*addressof*/ true); + return isInvalidICRSource(C, op->getSubExpr(), /*addressof*/ true, + isWeakAccess); // Skip certain casts. } else if (CastExpr *ce = dyn_cast(e)) { @@ -3862,7 +3863,7 @@ static InvalidICRKind isInvalidICRSource(ASTContext &C, Expr *e, case CK_BitCast: case CK_LValueBitCast: case CK_NoOp: - return isInvalidICRSource(C, ce->getSubExpr(), isAddressOf); + return isInvalidICRSource(C, ce->getSubExpr(), isAddressOf, isWeakAccess); case CK_ArrayToPointerDecay: return IIK_nonscalar; @@ -3876,6 +3877,11 @@ static InvalidICRKind isInvalidICRSource(ASTContext &C, Expr *e, // If we have a declaration reference, it had better be a local variable. } else if (isa(e)) { + // set isWeakAccess to true, to mean that there will be an implicit + // load which requires a cleanup. + if (e->getType().getObjCLifetime() == Qualifiers::OCL_Weak) + isWeakAccess = true; + if (!isAddressOf) return IIK_nonlocal; VarDecl *var = dyn_cast(cast(e)->getDecl()); @@ -3885,10 +3891,11 @@ static InvalidICRKind isInvalidICRSource(ASTContext &C, Expr *e, // If we have a conditional operator, check both sides. } else if (ConditionalOperator *cond = dyn_cast(e)) { - if (InvalidICRKind iik = isInvalidICRSource(C, cond->getLHS(), isAddressOf)) + if (InvalidICRKind iik = isInvalidICRSource(C, cond->getLHS(), isAddressOf, + isWeakAccess)) return iik; - return isInvalidICRSource(C, cond->getRHS(), isAddressOf); + return isInvalidICRSource(C, cond->getRHS(), isAddressOf, isWeakAccess); // These are never scalar. } else if (isa(e)) { @@ -3907,8 +3914,13 @@ static InvalidICRKind isInvalidICRSource(ASTContext &C, Expr *e, /// indirect copy/restore. static void checkIndirectCopyRestoreSource(Sema &S, Expr *src) { assert(src->isRValue()); - - InvalidICRKind iik = isInvalidICRSource(S.Context, src, false); + bool isWeakAccess = false; + InvalidICRKind iik = isInvalidICRSource(S.Context, src, false, isWeakAccess); + // If isWeakAccess to true, there will be an implicit + // load which requires a cleanup. + if (S.getLangOpts().ObjCAutoRefCount && isWeakAccess) + S.ExprNeedsCleanups = true; + if (iik == IIK_okay) return; S.Diag(src->getExprLoc(), diag::err_arc_nonlocal_writeback) diff --git a/test/CodeGenObjC/arc-blocks.m b/test/CodeGenObjC/arc-blocks.m index 63bb7a37c3..f87f9a4b23 100644 --- a/test/CodeGenObjC/arc-blocks.m +++ b/test/CodeGenObjC/arc-blocks.m @@ -250,7 +250,7 @@ void test7(void) { // 0x42800000 - has signature, copy/dispose helpers, as well as BLOCK_HAS_EXTENDED_LAYOUT // CHECK: store i32 -1040187392, // CHECK: [[SLOT:%.*]] = getelementptr inbounds [[BLOCK_T]]* [[BLOCK]], i32 0, i32 5 - // CHECK-NEXT: [[T0:%.*]] = call i8* @objc_loadWeak(i8** [[VAR]]) + // CHECK-NEXT: [[T0:%.*]] = call i8* @objc_loadWeakRetained(i8** [[VAR]]) // CHECK-NEXT: call i8* @objc_initWeak(i8** [[SLOT]], i8* [[T0]]) // CHECK: call void @test7_helper( // CHECK-NEXT: call void @objc_destroyWeak(i8** {{%.*}}) @@ -259,8 +259,9 @@ void test7(void) { // CHECK: define internal void @__test7_block_invoke // CHECK: [[SLOT:%.*]] = getelementptr inbounds [[BLOCK_T]]* {{%.*}}, i32 0, i32 5 - // CHECK-NEXT: [[T0:%.*]] = call i8* @objc_loadWeak(i8** [[SLOT]]) + // CHECK-NEXT: [[T0:%.*]] = call i8* @objc_loadWeakRetained(i8** [[SLOT]]) // CHECK-NEXT: call void @test7_consume(i8* [[T0]]) + // CHECK-NEXT: call void @objc_release(i8* [[T0]]) // CHECK-NEXT: ret void // CHECK: define internal void @__copy_helper_block_ diff --git a/test/CodeGenObjC/arc-foreach.m b/test/CodeGenObjC/arc-foreach.m index b8d2d30ab4..bb2485580c 100644 --- a/test/CodeGenObjC/arc-foreach.m +++ b/test/CodeGenObjC/arc-foreach.m @@ -109,8 +109,9 @@ void test1(NSArray *array) { // CHECK-LP64: [[D0:%.*]] = getelementptr inbounds [[BLOCK_T]]* [[BLOCK]], i32 0, i32 5 // CHECK-LP64: [[T0:%.*]] = getelementptr inbounds [[BLOCK_T]]* [[BLOCK]], i32 0, i32 5 -// CHECK-LP64-NEXT: [[T1:%.*]] = call i8* @objc_loadWeak(i8** [[X]]) +// CHECK-LP64-NEXT: [[T1:%.*]] = call i8* @objc_loadWeakRetained(i8** [[X]]) // CHECK-LP64-NEXT: call i8* @objc_initWeak(i8** [[T0]], i8* [[T1]]) +// CHECK-LP64-NEXT: call void @objc_release(i8* [[T1]]) // CHECK-LP64-NEXT: [[T1:%.*]] = bitcast [[BLOCK_T]]* [[BLOCK]] to // CHECK-LP64: call void @use_block // CHECK-LP64-NEXT: call void @objc_destroyWeak(i8** [[D0]]) diff --git a/test/CodeGenObjC/arc-loadweakretained-release.m b/test/CodeGenObjC/arc-loadweakretained-release.m new file mode 100644 index 0000000000..00d25fac0d --- /dev/null +++ b/test/CodeGenObjC/arc-loadweakretained-release.m @@ -0,0 +1,77 @@ +// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -fblocks -fobjc-arc -fobjc-runtime-has-weak -o - %s | FileCheck %s +// rdar://10849570 + +@interface NSObject @end + +@interface SomeClass : NSObject +- (id) init; +@end + +@implementation SomeClass +- (void)foo { +} +- (id) init { + return 0; +} ++ alloc { return 0; } +@end + +int main (int argc, const char * argv[]) { + @autoreleasepool { + SomeClass *objPtr1 = [[SomeClass alloc] init]; + __weak SomeClass *weakRef = objPtr1; + + [weakRef foo]; + + objPtr1 = (void *)0; + return 0; + } +} + +// CHECK: [[SIXTEEN:%.*]] = call i8* @objc_loadWeakRetained(i8** {{%.*}}) +// CHECK-NEXT: [[SEVENTEEN:%.*]] = bitcast i8* [[SIXTEEN]] to {{%.*}} +// CHECK-NEXT: [[EIGHTEEN:%.*]] = load i8** @"\01L_OBJC_SELECTOR_REFERENCES_6" +// CHECK-NEXT: [[NINETEEN:%.*]] = bitcast %0* [[SEVENTEEN]] to i8* +// CHECK-NEXT: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend +// CHECK-NEXT: [[TWENTY:%.*]] = bitcast %0* [[SEVENTEEN]] to i8* +// CHECK-NEXT: call void @objc_release(i8* [[TWENTY]]) + +void test1(int cond) { + extern void test34_sink(id *); + __weak id weak; + test34_sink(cond ? &weak : 0); +} + +// CHECK: define void @test1( +// CHECK: [[CONDADDR:%.*]] = alloca i32 +// CHECK-NEXT: [[WEAK:%.*]] = alloca i8* +// CHECK-NEXT: [[INCRTEMP:%.*]] = alloca i8* +// CHECK-NEXT: [[CONDCLEANUPSAVE:%.*]] = alloca i8* +// CHECK-NEXT: [[CONDCLEANUP:%.*]] = alloca i1 +// CHECK-NEXT: store i32 +// CHECK-NEXT: store i8* null, i8** [[WEAK]] +// CHECK: [[COND1:%.*]] = phi i8** +// CHECK-NEXT: [[ICRISNULL:%.*]] = icmp eq i8** [[COND1]], null +// CHECK-NEXT: [[ICRARGUMENT:%.*]] = select i1 [[ICRISNULL]], i8** null, i8** [[INCRTEMP]] +// CHECK-NEXT: store i1 false, i1* [[CONDCLEANUP]] +// CHECK-NEXT: br i1 [[ICRISNULL]], label [[ICRCONT:%.*]], label [[ICRCOPY:%.*]] +// CHECK: [[ONE:%.*]] = call i8* @objc_loadWeakRetained( +// CHECK-NEXT: store i8* [[ONE]], i8** [[CONDCLEANUPSAVE]] +// CHECK-NEXT: store i1 true, i1* [[CONDCLEANUP]] +// CHECK-NEXT: store i8* [[ONE]], i8** [[INCRTEMP]] +// CHECK-NEXT: br label + +// CHECK: call void @test34_sink( +// CHECK-NEXT: [[ICRISNULL1:%.*]] = icmp eq i8** [[COND1]], null +// CHECK-NEXT: br i1 [[ICRISNULL1]], label [[ICRDONE:%.*]], label [[ICRWRITEBACK:%.*]] +// CHECK: [[TWO:%.*]] = load i8** [[INCRTEMP]] +// CHECK-NEXT: [[THREE:%.*]] = call i8* @objc_storeWeak( +// CHECK-NEXT br label [[ICRDONE]] +// CHECK: [[CLEANUPISACTIVE:%.*]] = load i1* [[CONDCLEANUP]] +// CHECK-NEXT: br i1 [[CLEANUPISACTIVE]], label [[CLEASNUPACTION:%.*]], label [[CLEANUPDONE:%.*]] + +// CHECK: [[FOUR:%.*]] = load i8** [[CONDCLEANUPSAVE]] +// CHECK-NEXT: call void @objc_release(i8* [[FOUR]]) +// CHECK-NEXT: br label +// CHECK: call void @objc_destroyWeak(i8** [[WEAK]]) +// CHECK-NEXT: ret void diff --git a/test/CodeGenObjC/arc.m b/test/CodeGenObjC/arc.m index 8e38019de5..43a7a30967 100644 --- a/test/CodeGenObjC/arc.m +++ b/test/CodeGenObjC/arc.m @@ -958,6 +958,8 @@ void test34(int cond) { // CHECK-NEXT: [[WEAK:%.*]] = alloca i8* // CHECK-NEXT: [[TEMP1:%.*]] = alloca i8* // CHECK-NEXT: [[TEMP2:%.*]] = alloca i8* + // CHECK-NEXT: [[CONDCLEANUPSAVE:%.*]] = alloca i8* + // CHECK-NEXT: [[CONDCLEANUP:%.*]] = alloca i1 // CHECK-NEXT: store i32 // CHECK-NEXT: store i8* null, i8** [[STRONG]] // CHECK-NEXT: call i8* @objc_initWeak(i8** [[WEAK]], i8* null) @@ -986,8 +988,11 @@ void test34(int cond) { // CHECK: [[ARG:%.*]] = phi i8** // CHECK-NEXT: [[T0:%.*]] = icmp eq i8** [[ARG]], null // CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i8** null, i8** [[TEMP2]] + // CHECK-NEXT: store i1 false, i1* [[CONDCLEANUP]] // CHECK-NEXT: br i1 [[T0]], - // CHECK: [[T0:%.*]] = call i8* @objc_loadWeak(i8** [[ARG]]) + // CHECK: [[T0:%.*]] = call i8* @objc_loadWeakRetained(i8** [[ARG]]) + // CHECK-NEXT: store i8* [[T0]], i8** [[CONDCLEANUPSAVE]] + // CHECK-NEXT: store i1 true, i1* [[CONDCLEANUP]] // CHECK-NEXT: store i8* [[T0]], i8** [[TEMP2]] // CHECK-NEXT: br label // CHECK: call void @test34_sink(i8** [[T1]]) diff --git a/test/CodeGenObjCXX/arc.mm b/test/CodeGenObjCXX/arc.mm index f31b993946..aa40f930ca 100644 --- a/test/CodeGenObjCXX/arc.mm +++ b/test/CodeGenObjCXX/arc.mm @@ -61,6 +61,8 @@ void test34(int cond) { // CHECK-NEXT: [[WEAK:%.*]] = alloca i8* // CHECK-NEXT: [[TEMP1:%.*]] = alloca i8* // CHECK-NEXT: [[TEMP2:%.*]] = alloca i8* + // CHECK-NEXT: [[CONDCLEANUPSAVE:%.*]] = alloca i8* + // CHECK-NEXT: [[CONDCLEANUP:%.*]] = alloca i1 // CHECK-NEXT: store i32 // CHECK-NEXT: store i8* null, i8** [[STRONG]] // CHECK-NEXT: call i8* @objc_initWeak(i8** [[WEAK]], i8* null) @@ -89,8 +91,11 @@ void test34(int cond) { // CHECK: [[ARG:%.*]] = phi i8** // CHECK-NEXT: [[T0:%.*]] = icmp eq i8** [[ARG]], null // CHECK-NEXT: [[T1:%.*]] = select i1 [[T0]], i8** null, i8** [[TEMP2]] + // CHECK-NEXT: store i1 false, i1* [[CONDCLEANUP]] // CHECK-NEXT: br i1 [[T0]], - // CHECK: [[T0:%.*]] = call i8* @objc_loadWeak(i8** [[ARG]]) + // CHECK: [[T0:%.*]] = call i8* @objc_loadWeakRetained(i8** [[ARG]]) + // CHECK-NEXT: store i8* [[T0]], i8** [[CONDCLEANUPSAVE]] + // CHECK-NEXT: store i1 true, i1* [[CONDCLEANUP]] // CHECK-NEXT: store i8* [[T0]], i8** [[TEMP2]] // CHECK-NEXT: br label // CHECK: call void @_Z11test34_sinkPU15__autoreleasingP11objc_object(i8** [[T1]]) -- 2.40.0