From 7acddacc921cd0b3f813443a8641eeddb82dfbd4 Mon Sep 17 00:00:00 2001 From: John McCall Date: Fri, 17 Jun 2011 06:42:21 +0000 Subject: [PATCH] Objective-C fast enumeration loop variables are not retained in ARC, but they should still be officially __strong for the purposes of errors, block capture, etc. Make a new bit on variables, isARCPseudoStrong(), and set this for 'self' and these enumeration-loop variables. Change the code that was looking for the old patterns to look for this bit, and change IR generation to find this bit and treat the resulting variable as __unsafe_unretained for the purposes of init/destroy in the two places it can come up. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@133243 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Decl.h | 13 ++++- lib/ARCMigrate/Transforms.cpp | 5 +- lib/AST/DeclObjC.cpp | 12 +++-- lib/CodeGen/CGDecl.cpp | 42 +++++++++++++-- lib/CodeGen/CGObjC.cpp | 11 ++-- lib/CodeGen/CodeGenFunction.h | 1 + lib/Sema/SemaExpr.cpp | 23 ++++---- lib/Sema/SemaStmt.cpp | 31 +++++------ lib/Serialization/ASTReaderDecl.cpp | 1 + lib/Serialization/ASTWriterDecl.cpp | 7 ++- test/CodeGenObjC/arc-foreach.m | 81 ++++++++++++++++++++++------- test/SemaObjC/arc.m | 2 +- 12 files changed, 169 insertions(+), 60 deletions(-) diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h index d3fe6b8c48..2cc8ec16ff 100644 --- a/include/clang/AST/Decl.h +++ b/include/clang/AST/Decl.h @@ -702,8 +702,12 @@ private: /// \brief Whether this variable is the for-range-declaration in a C++0x /// for-range statement. unsigned CXXForRangeDecl : 1; + + /// \brief Whether this variable is an ARC pseudo-__strong + /// variable; see isARCPseudoStrong() for details. + unsigned ARCPseudoStrong : 1; }; - enum { NumVarDeclBits = 13 }; // two reserved bits for now + enum { NumVarDeclBits = 13 }; // one reserved bit friend class ASTDeclReader; friend class StmtIteratorBase; @@ -1102,6 +1106,13 @@ public: /// a C++0x for-range statement. bool isCXXForRangeDecl() const { return VarDeclBits.CXXForRangeDecl; } void setCXXForRangeDecl(bool FRD) { VarDeclBits.CXXForRangeDecl = FRD; } + + /// \brief Determine whether this variable is an ARC pseudo-__strong + /// variable. A pseudo-__strong variable has a __strong-qualified + /// type but does not actually retain the object written into it. + /// Generally such variables are also 'const' for safety. + bool isARCPseudoStrong() const { return VarDeclBits.ARCPseudoStrong; } + void setARCPseudoStrong(bool ps) { VarDeclBits.ARCPseudoStrong = ps; } /// \brief If this variable is an instantiated static data member of a /// class template specialization, returns the templated static data member diff --git a/lib/ARCMigrate/Transforms.cpp b/lib/ARCMigrate/Transforms.cpp index ab3bc1dbe7..0c9096296d 100644 --- a/lib/ARCMigrate/Transforms.cpp +++ b/lib/ARCMigrate/Transforms.cpp @@ -421,10 +421,7 @@ public: if (IsLV != Expr::MLV_ConstQualified) return true; VarDecl *var = cast(declRef->getDecl()); - if (var->getType().getLocalQualifiers().getObjCLifetime() - == Qualifiers::OCL_ExplicitNone && - (var->getTypeSourceInfo() && - !var->getTypeSourceInfo()->getType().isConstQualified())) { + if (var->isARCPseudoStrong()) { Transaction Trans(Pass.TA); if (Pass.TA.clearDiagnostic(diag::err_typecheck_arr_assign_enumeration, Exp->getOperatorLoc())) { diff --git a/lib/AST/DeclObjC.cpp b/lib/AST/DeclObjC.cpp index 99eb0d386b..67f6531d79 100644 --- a/lib/AST/DeclObjC.cpp +++ b/lib/AST/DeclObjC.cpp @@ -474,19 +474,22 @@ void ObjCMethodDecl::createImplicitParams(ASTContext &Context, } else // we have a factory method. selfTy = Context.getObjCClassType(); + bool selfIsPseudoStrong = false; bool selfIsConsumed = false; if (isInstanceMethod() && Context.getLangOptions().ObjCAutoRefCount) { selfIsConsumed = hasAttr(); - // 'self' is always __strong, although as a special case we don't - // actually retain it except in init methods. + // 'self' is always __strong. It's actually pseudo-strong except + // in init methods, though. Qualifiers qs; qs.setObjCLifetime(Qualifiers::OCL_Strong); selfTy = Context.getQualifiedType(selfTy, qs); // In addition, 'self' is const unless this is an init method. - if (getMethodFamily() != OMF_init) + if (getMethodFamily() != OMF_init) { selfTy = selfTy.withConst(); + selfIsPseudoStrong = true; + } } ImplicitParamDecl *self @@ -497,6 +500,9 @@ void ObjCMethodDecl::createImplicitParams(ASTContext &Context, if (selfIsConsumed) self->addAttr(new (Context) NSConsumedAttr(SourceLocation(), Context)); + if (selfIsPseudoStrong) + self->setARCPseudoStrong(true); + setCmdDecl(ImplicitParamDecl::Create(Context, this, SourceLocation(), &Context.Idents.get("_cmd"), Context.getObjCSelType())); diff --git a/lib/CodeGen/CGDecl.cpp b/lib/CodeGen/CGDecl.cpp index daa37eea8f..a79f031ec0 100644 --- a/lib/CodeGen/CGDecl.cpp +++ b/lib/CodeGen/CGDecl.cpp @@ -562,6 +562,37 @@ void CodeGenFunction::EmitScalarInit(const Expr *init, EmitStoreOfScalar(value, lvalue); } +/// EmitScalarInit - Initialize the given lvalue with the given object. +void CodeGenFunction::EmitScalarInit(llvm::Value *init, LValue lvalue) { + Qualifiers::ObjCLifetime lifetime = lvalue.getObjCLifetime(); + if (!lifetime) + return EmitStoreThroughLValue(RValue::get(init), lvalue, lvalue.getType()); + + switch (lifetime) { + case Qualifiers::OCL_None: + llvm_unreachable("present but none"); + + case Qualifiers::OCL_ExplicitNone: + // nothing to do + break; + + case Qualifiers::OCL_Strong: + init = EmitARCRetain(lvalue.getType(), init); + break; + + case Qualifiers::OCL_Weak: + // Initialize and then skip the primitive store. + EmitARCInitWeak(lvalue.getAddress(), init); + return; + + case Qualifiers::OCL_Autoreleasing: + init = EmitARCRetainAutorelease(lvalue.getType(), init); + break; + } + + EmitStoreOfScalar(init, lvalue); +} + /// canEmitInitWithFewStoresAfterMemset - Decide whether we can emit the /// non-zero parts of the specified initializer with equal or fewer than /// NumStores scalar stores. @@ -995,8 +1026,10 @@ void CodeGenFunction::EmitAutoVarCleanups(const AutoVarEmission &emission) { if (Qualifiers::ObjCLifetime lifetime = D.getType().getQualifiers().getObjCLifetime()) { - llvm::Value *loc = emission.getObjectAddress(*this); - EmitAutoVarWithLifetime(*this, D, loc, lifetime); + if (!D.isARCPseudoStrong()) { + llvm::Value *loc = emission.getObjectAddress(*this); + EmitAutoVarWithLifetime(*this, D, loc, lifetime); + } } // Handle the cleanup attribute. @@ -1081,10 +1114,11 @@ void CodeGenFunction::EmitParmDecl(const VarDecl &D, llvm::Value *Arg, // 'self' is always formally __strong, but if this is not an // init method then we don't want to retain it. - if (lt == Qualifiers::OCL_Strong && qs.hasConst() && - isa(D)) { + if (D.isARCPseudoStrong()) { const ObjCMethodDecl *method = cast(CurCodeDecl); assert(&D == method->getSelfDecl()); + assert(lt == Qualifiers::OCL_Strong); + assert(qs.hasConst()); assert(method->getMethodFamily() != OMF_init); (void) method; lt = Qualifiers::OCL_ExplicitNone; diff --git a/lib/CodeGen/CGObjC.cpp b/lib/CodeGen/CGObjC.cpp index cdc2fffd94..ce9c262fd8 100644 --- a/lib/CodeGen/CGObjC.cpp +++ b/lib/CodeGen/CGObjC.cpp @@ -1110,6 +1110,9 @@ void CodeGenFunction::EmitObjCForCollectionStmt(const ObjCForCollectionStmt &S){ elementLValue = EmitLValue(&tempDRE); elementType = D->getType(); elementIsVariable = true; + + if (D->isARCPseudoStrong()) + elementLValue.getQuals().setObjCLifetime(Qualifiers::OCL_ExplicitNone); } else { elementLValue = LValue(); // suppress warning elementType = cast(S.getElement())->getType(); @@ -1136,10 +1139,12 @@ void CodeGenFunction::EmitObjCForCollectionStmt(const ObjCForCollectionStmt &S){ // Make sure we have an l-value. Yes, this gets evaluated every // time through the loop. - if (!elementIsVariable) + if (!elementIsVariable) { elementLValue = EmitLValue(cast(S.getElement())); - - EmitStoreThroughLValue(RValue::get(CurrentItem), elementLValue, elementType); + EmitStoreThroughLValue(RValue::get(CurrentItem), elementLValue, elementType); + } else { + EmitScalarInit(CurrentItem, elementLValue); + } // If we do have an element variable, this assignment is the end of // its initialization. diff --git a/lib/CodeGen/CodeGenFunction.h b/lib/CodeGen/CodeGenFunction.h index dec7736bdc..f34a70c5d9 100644 --- a/lib/CodeGen/CodeGenFunction.h +++ b/lib/CodeGen/CodeGenFunction.h @@ -1597,6 +1597,7 @@ public: void EmitScalarInit(const Expr *init, const ValueDecl *D, LValue lvalue, bool capturedByInit); + void EmitScalarInit(llvm::Value *init, LValue lvalue); typedef void SpecialInitFn(CodeGenFunction &Init, const VarDecl &D, llvm::Value *Address); diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 2d8ed17b70..e3fde7cfc9 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -8161,25 +8161,28 @@ static bool CheckForModifiableLvalue(Expr *E, SourceLocation Loc, Sema &S) { case Expr::MLV_ConstQualified: Diag = diag::err_typecheck_assign_const; - // In ARC, use some specialized diagnostics for the times when we - // infer const. + // In ARC, use some specialized diagnostics for occasions where we + // infer 'const'. These are always pseudo-strong variables. if (S.getLangOptions().ObjCAutoRefCount) { DeclRefExpr *declRef = dyn_cast(E->IgnoreParenCasts()); if (declRef && isa(declRef->getDecl())) { VarDecl *var = cast(declRef->getDecl()); - // If the variable wasn't written with 'const', there are some - // cases where we infer const anyway: - // - self - // - fast enumeration variables - if (!var->getTypeSourceInfo() || - !var->getTypeSourceInfo()->getType().isConstQualified()) { + // Use the normal diagnostic if it's pseudo-__strong but the + // user actually wrote 'const'. + if (var->isARCPseudoStrong() && + (!var->getTypeSourceInfo() || + !var->getTypeSourceInfo()->getType().isConstQualified())) { + // There are two pseudo-strong cases: + // - self ObjCMethodDecl *method = S.getCurMethodDecl(); if (method && var == method->getSelfDecl()) Diag = diag::err_typecheck_arr_assign_self; - else if (var->getType().getObjCLifetime() - == Qualifiers::OCL_ExplicitNone) + + // - fast enumeration variables + else Diag = diag::err_typecheck_arr_assign_enumeration; + SourceRange Assign; if (Loc != OrigLoc) Assign = SourceRange(OrigLoc, OrigLoc); diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index a5ad0a5a2b..9af9c8de1b 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -71,22 +71,23 @@ void Sema::ActOnForEachDeclStmt(DeclGroupPtrTy dg) { // suppress any potential 'unused variable' warning. var->setUsed(); - // In ARC, we don't want to lifetime for the iteration - // variable of a fast enumeration loop. Rather than actually - // trying to catch that during declaration processing, we - // remove the consequences here. - if (getLangOptions().ObjCAutoRefCount) { - SplitQualType split = var->getType().split(); + // foreach variables are never actually initialized in the way that + // the parser came up with. + var->setInit(0); - // Inferred lifetime will show up as a local qualifier because - // explicit lifetime would have shown up as an AttributedType - // instead. - if (split.second.hasObjCLifetime()) { - // Change the qualification to 'const __unsafe_unretained'. - split.second.setObjCLifetime(Qualifiers::OCL_ExplicitNone); - split.second.addConst(); - var->setType(Context.getQualifiedType(split.first, split.second)); - var->setInit(0); + // In ARC, we don't need to retain the iteration variable of a fast + // enumeration loop. Rather than actually trying to catch that + // during declaration processing, we remove the consequences here. + if (getLangOptions().ObjCAutoRefCount) { + QualType type = var->getType(); + + // Only do this if we inferred the lifetime. Inferred lifetime + // will show up as a local qualifier because explicit lifetime + // should have shown up as an AttributedType instead. + if (type.getLocalQualifiers().getObjCLifetime() == Qualifiers::OCL_Strong) { + // Add 'const' and mark the variable as pseudo-strong. + var->setType(type.withConst()); + var->setARCPseudoStrong(true); } } } diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index fab2069bc5..24ab544dcd 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -710,6 +710,7 @@ void ASTDeclReader::VisitVarDecl(VarDecl *VD) { VD->VarDeclBits.ExceptionVar = Record[Idx++]; VD->VarDeclBits.NRVOVariable = Record[Idx++]; VD->VarDeclBits.CXXForRangeDecl = Record[Idx++]; + VD->VarDeclBits.ARCPseudoStrong = Record[Idx++]; if (Record[Idx++]) VD->setInit(Reader.ReadExpr(F)); diff --git a/lib/Serialization/ASTWriterDecl.cpp b/lib/Serialization/ASTWriterDecl.cpp index 7c24088c65..651e2078cf 100644 --- a/lib/Serialization/ASTWriterDecl.cpp +++ b/lib/Serialization/ASTWriterDecl.cpp @@ -645,6 +645,7 @@ void ASTDeclWriter::VisitVarDecl(VarDecl *D) { Record.push_back(D->isExceptionVariable()); Record.push_back(D->isNRVOVariable()); Record.push_back(D->isCXXForRangeDecl()); + Record.push_back(D->isARCPseudoStrong()); Record.push_back(D->getInit() ? 1 : 0); if (D->getInit()) Writer.AddStmt(D->getInit()); @@ -670,7 +671,7 @@ void ASTDeclWriter::VisitVarDecl(VarDecl *D) { D->RedeclLink.getNext() == D && !D->hasCXXDirectInitializer() && D->getInit() == 0 && - !ParmVarDecl::classofKind(D->getKind()) && + !isa(D) && !SpecInfo) AbbrevToUse = Writer.getDeclVarAbbrev(); @@ -695,6 +696,8 @@ void ASTDeclWriter::VisitParmVarDecl(ParmVarDecl *D) { Writer.AddStmt(D->getUninstantiatedDefaultArg()); Code = serialization::DECL_PARM_VAR; + assert(!D->isARCPseudoStrong()); // can be true of ImplicitParamDecl + // If the assumptions about the DECL_PARM_VAR abbrev are true, use it. Here // we dynamically check for the properties that we optimize for, but don't // know are true of all PARM_VAR_DECLs. @@ -1426,6 +1429,7 @@ void ASTWriter::WriteDeclsBlockAbbrevs() { Abv->Add(BitCodeAbbrevOp(0)); // isExceptionVariable Abv->Add(BitCodeAbbrevOp(0)); // isNRVOVariable Abv->Add(BitCodeAbbrevOp(0)); // isCXXForRangeDecl + Abv->Add(BitCodeAbbrevOp(0)); // isARCPseudoStrong Abv->Add(BitCodeAbbrevOp(0)); // HasInit Abv->Add(BitCodeAbbrevOp(0)); // HasMemberSpecializationInfo // ParmVarDecl @@ -1498,6 +1502,7 @@ void ASTWriter::WriteDeclsBlockAbbrevs() { Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isExceptionVariable Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isNRVOVariable Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isCXXForRangeDecl + Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // isARCPseudoStrong Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // HasInit Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // HasMemberSpecInfo // Type Source Info diff --git a/test/CodeGenObjC/arc-foreach.m b/test/CodeGenObjC/arc-foreach.m index 00b0adf393..e8a5b8851e 100644 --- a/test/CodeGenObjC/arc-foreach.m +++ b/test/CodeGenObjC/arc-foreach.m @@ -1,27 +1,72 @@ // RUN: %clang_cc1 -fblocks -fobjc-arc -fobjc-nonfragile-abi -triple x86_64-apple-darwin -O0 -emit-llvm %s -o %t-64.s // RUN: FileCheck -check-prefix LP64 --input-file=%t-64.s %s // rdar://9503326 +// rdar://9606600 -typedef void (^dispatch_block_t)(void); - -@class NSString; -extern void NSLog(NSString *format, ...) __attribute__((format(__NSString__, 1, 2))); +extern void use(id); +extern void use_block(void (^)(void)); @class NSArray; -int main (int argc, const char * argv[]) -{ - NSArray *array; - for ( NSString *str in array) { - dispatch_block_t blk = ^{ - NSLog(@"str in block: %@", str); - }; - blk(); - } - return 0; +void test0(NSArray *array) { + // 'x' should be initialized without a retain. + // We should actually do a non-constant capture, and that + // capture should require a retain. + for (id x in array) { + use_block(^{ use(x); }); + } } -// CHECK-LP64: define internal void @__main_block_invoke -// CHECK-LP64: [[BLOCK:%.*]] = bitcast i8* {{%.*}} to [[BLOCK_T:%.*]]* +// CHECK-LP64: define void @test0( +// CHECK-LP64: alloca [[ARRAY_T:%.*]]*, +// CHECK-LP64-NEXT: [[X:%.*]] = alloca i8*, +// CHECK-LP64-NEXT: [[STATE:%.*]] = alloca [[STATE_T:%.*]], +// CHECK-LP64-NEXT: alloca [16 x i8*], align 8 +// CHECK-LP64-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:%.*]], + +// CHECK-LP64: [[T0:%.*]] = getelementptr inbounds [[STATE_T]]* [[STATE]], i32 0, i32 1 +// CHECK-LP64-NEXT: [[T1:%.*]] = load i8*** [[T0]] +// CHECK-LP64-NEXT: [[T2:%.*]] = getelementptr i8** [[T1]], i64 +// CHECK-LP64-NEXT: [[T3:%.*]] = load i8** [[T2]] +// CHECK-LP64-NEXT: store i8* [[T3]], i8** [[X]] + +// CHECK-LP64: [[T0:%.*]] = getelementptr inbounds [[BLOCK_T]]* [[BLOCK]], i32 0, i32 5 +// CHECK-LP64-NEXT: [[T1:%.*]] = load i8** [[X]] +// CHECK-LP64-NEXT: [[T2:%.*]] = call i8* @objc_retain(i8* [[T1]]) +// CHECK-LP64-NEXT: store i8* [[T2]], i8** [[T0]] +// CHECK-LP64-NEXT: [[T1:%.*]] = bitcast [[BLOCK_T]]* [[BLOCK]] to void ()* +// CHECK-LP64-NEXT: call void @use_block(void ()* [[T1]]) +// CHECK-LP64-NEXT: [[T1:%.*]] = load i8** [[T0]] +// CHECK-LP64-NEXT: call void @objc_release(i8* [[T1]]) + +// CHECK-LP64: define internal void @__test0_block_invoke +// CHECK-LP64: [[BLOCK:%.*]] = bitcast i8* {{%.*}} to [[BLOCK_T]]* // CHECK-LP64-NEXT: [[T0:%.*]] = getelementptr inbounds [[BLOCK_T]]* [[BLOCK]], i32 0, i32 5 -// CHECK-LP64-NEXT: [[T2:%.*]] = load [[OPAQUE_T:%.*]]** [[T0]], align 8 -// CHECK-LP64-NEXT: call void ([[OPAQUE_T]]*, ...)* @NSLog +// CHECK-LP64-NEXT: [[T2:%.*]] = load i8** [[T0]], align 8 +// CHECK-LP64-NEXT: call void @use(i8* [[T2]]) + +void test1(NSArray *array) { + for (__weak id x in array) { + use_block(^{ use(x); }); + } +} + +// CHECK-LP64: define void @test1( +// CHECK-LP64: alloca [[ARRAY_T:%.*]]*, +// CHECK-LP64-NEXT: [[X:%.*]] = alloca i8*, +// CHECK-LP64-NEXT: [[STATE:%.*]] = alloca [[STATE_T:%.*]], +// CHECK-LP64-NEXT: alloca [16 x i8*], align 8 +// CHECK-LP64-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:%.*]], + +// CHECK-LP64: [[T0:%.*]] = getelementptr inbounds [[STATE_T]]* [[STATE]], i32 0, i32 1 +// CHECK-LP64-NEXT: [[T1:%.*]] = load i8*** [[T0]] +// CHECK-LP64-NEXT: [[T2:%.*]] = getelementptr i8** [[T1]], i64 +// CHECK-LP64-NEXT: [[T3:%.*]] = load i8** [[T2]] +// CHECK-LP64-NEXT: call i8* @objc_initWeak(i8** [[X]], i8* [[T3]]) + +// 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: call i8* @objc_initWeak(i8** [[T0]], i8* [[T1]]) +// CHECK-LP64-NEXT: [[T1:%.*]] = bitcast [[BLOCK_T]]* [[BLOCK]] to void ()* +// CHECK-LP64-NEXT: call void @use_block(void ()* [[T1]]) +// CHECK-LP64-NEXT: call void @objc_destroyWeak(i8** [[T0]]) +// CHECK-LP64-NEXT: call void @objc_destroyWeak(i8** [[X]]) diff --git a/test/SemaObjC/arc.m b/test/SemaObjC/arc.m index 4c2ae236ac..d867b7332e 100644 --- a/test/SemaObjC/arc.m +++ b/test/SemaObjC/arc.m @@ -241,7 +241,7 @@ id test9(Test9 *v) { // Test that the inference rules are different for fast enumeration variables. void test10(id collection) { for (id x in collection) { - __strong id *ptr = &x; // expected-error {{initializing '__strong id *' with an expression of type 'const __unsafe_unretained id *' changes retain/release properties of pointer}} + __strong id *ptr = &x; // expected-warning {{initializing '__strong id *' with an expression of type 'const __strong id *' discards qualifiers}} } for (__strong id x in collection) {