From dc05b11c67331016473fbc7909827b1b89c9616b Mon Sep 17 00:00:00 2001 From: John McCall Date: Sat, 10 Sep 2011 01:16:55 +0000 Subject: [PATCH] When converting a block pointer to an Objective-C pointer type, extend the lifetime of the block by copying it to the heap, or else we'll get a dangling reference because the code working with the non-block-typed object will not know it needs to copy. There is some danger here, e.g. with assigning a block literal to an unsafe variable, but, well, it's an unsafe variable. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@139451 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/OperationKinds.h | 25 +++++---- include/clang/Sema/Sema.h | 1 + lib/AST/Expr.cpp | 3 ++ lib/AST/ExprConstant.cpp | 2 + lib/CodeGen/CGExpr.cpp | 3 +- lib/CodeGen/CGExprAgg.cpp | 1 + lib/CodeGen/CGExprComplex.cpp | 1 + lib/CodeGen/CGExprConstant.cpp | 1 + lib/CodeGen/CGExprScalar.cpp | 6 ++- lib/CodeGen/CGObjC.cpp | 70 +++++++++++++++++++++++++ lib/Sema/SemaCXXCast.cpp | 8 +-- lib/Sema/SemaExpr.cpp | 34 +++++++++++- lib/Sema/SemaExprCXX.cpp | 9 ++++ lib/StaticAnalyzer/Core/ExprEngineC.cpp | 3 +- test/CodeGenObjC/arc.m | 37 +++++++++++++ 15 files changed, 184 insertions(+), 20 deletions(-) diff --git a/include/clang/AST/OperationKinds.h b/include/clang/AST/OperationKinds.h index fa9adec4ef..677642213b 100644 --- a/include/clang/AST/OperationKinds.h +++ b/include/clang/AST/OperationKinds.h @@ -254,20 +254,27 @@ enum CastKind { /// _Complex unsigned -> _Complex float CK_IntegralComplexToFloatingComplex, - /// \brief Produces a retainable object pointer so that it may be - /// consumed, e.g. by being passed to a consuming parameter. Calls - /// objc_retain. + /// \brief [ARC] Produces a retainable object pointer so that it may + /// be consumed, e.g. by being passed to a consuming parameter. + /// Calls objc_retain. CK_ObjCProduceObject, - /// \brief Consumes a retainable object pointer that has just been - /// produced, e.g. as the return value of a retaining call. Enters - /// a cleanup to call objc_release at some indefinite time. + /// \brief [ARC] Consumes a retainable object pointer that has just + /// been produced, e.g. as the return value of a retaining call. + /// Enters a cleanup to call objc_release at some indefinite time. CK_ObjCConsumeObject, - /// \brief Reclaim a retainable object pointer object that may have - /// been produced and autoreleased as part of a function return + /// \brief [ARC] Reclaim a retainable object pointer object that may + /// have been produced and autoreleased as part of a function return /// sequence. - CK_ObjCReclaimReturnedObject + CK_ObjCReclaimReturnedObject, + + /// \brief [ARC] Causes a value of block type to be copied to the + /// heap, if it is not already there. A number of other operations + /// in ARC cause blocks to be copied; this is for cases where that + /// would not otherwise be guaranteed, such as when casting to a + /// non-block pointer type. + CK_ObjCExtendBlockObject }; #define CK_Invalid ((CastKind) -1) diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 4e61fd1744..29c7141880 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -1357,6 +1357,7 @@ public: bool FunctionArgTypesAreEqual(const FunctionProtoType *OldType, const FunctionProtoType *NewType); + CastKind PrepareCastToObjCObjectPointer(ExprResult &E); bool CheckPointerConversion(Expr *From, QualType ToType, CastKind &Kind, CXXCastPath& BasePath, diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp index 25e7de77fb..63642b216f 100644 --- a/lib/AST/Expr.cpp +++ b/lib/AST/Expr.cpp @@ -1079,6 +1079,7 @@ void CastExpr::CheckCastConsistency() const { case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: assert(!getType()->isBooleanType() && "unheralded conversion to bool"); goto CheckNoBasePath; @@ -1198,6 +1199,8 @@ const char *CastExpr::getCastKindName() const { return "ObjCProduceObject"; case CK_ObjCReclaimReturnedObject: return "ObjCReclaimReturnedObject"; + case CK_ObjCExtendBlockObject: + return "ObjCExtendBlockObject"; } llvm_unreachable("Unhandled cast kind!"); diff --git a/lib/AST/ExprConstant.cpp b/lib/AST/ExprConstant.cpp index eb35dd7c98..7ae4186423 100644 --- a/lib/AST/ExprConstant.cpp +++ b/lib/AST/ExprConstant.cpp @@ -1830,6 +1830,7 @@ bool IntExprEvaluator::VisitCastExpr(const CastExpr *E) { case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: return false; case CK_LValueToRValue: @@ -2338,6 +2339,7 @@ bool ComplexExprEvaluator::VisitCastExpr(const CastExpr *E) { case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: llvm_unreachable("invalid cast kind for complex value"); case CK_LValueToRValue: diff --git a/lib/CodeGen/CGExpr.cpp b/lib/CodeGen/CGExpr.cpp index 7ae95331ad..520b1bbfb8 100644 --- a/lib/CodeGen/CGExpr.cpp +++ b/lib/CodeGen/CGExpr.cpp @@ -2061,7 +2061,8 @@ LValue CodeGenFunction::EmitCastLValue(const CastExpr *E) { case CK_AnyPointerToBlockPointerCast: case CK_ObjCProduceObject: case CK_ObjCConsumeObject: - case CK_ObjCReclaimReturnedObject: { + case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: { // These casts only produce lvalues when we're binding a reference to a // temporary realized from a (converted) pure rvalue. Emit the expression // as a value, copy it into a temporary, and return an lvalue referring to diff --git a/lib/CodeGen/CGExprAgg.cpp b/lib/CodeGen/CGExprAgg.cpp index fe6dfcd28f..93c7bd8af6 100644 --- a/lib/CodeGen/CGExprAgg.cpp +++ b/lib/CodeGen/CGExprAgg.cpp @@ -382,6 +382,7 @@ void AggExprEmitter::VisitCastExpr(CastExpr *E) { case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: llvm_unreachable("cast kind invalid for aggregate types"); } } diff --git a/lib/CodeGen/CGExprComplex.cpp b/lib/CodeGen/CGExprComplex.cpp index 90e288a450..326fcfa15b 100644 --- a/lib/CodeGen/CGExprComplex.cpp +++ b/lib/CodeGen/CGExprComplex.cpp @@ -414,6 +414,7 @@ ComplexPairTy ComplexExprEmitter::EmitCast(CastExpr::CastKind CK, Expr *Op, case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: llvm_unreachable("invalid cast kind for complex value"); case CK_FloatingRealToComplex: diff --git a/lib/CodeGen/CGExprConstant.cpp b/lib/CodeGen/CGExprConstant.cpp index b51ff38656..7ab77709e0 100644 --- a/lib/CodeGen/CGExprConstant.cpp +++ b/lib/CodeGen/CGExprConstant.cpp @@ -589,6 +589,7 @@ public: case CK_ObjCProduceObject: case CK_ObjCConsumeObject: case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: return 0; // These might need to be supported for constexpr. diff --git a/lib/CodeGen/CGExprScalar.cpp b/lib/CodeGen/CGExprScalar.cpp index 35876d8c31..6741f43366 100644 --- a/lib/CodeGen/CGExprScalar.cpp +++ b/lib/CodeGen/CGExprScalar.cpp @@ -1027,7 +1027,7 @@ Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { ConvertType(CGF.getContext().getPointerType(DestTy))); return EmitLoadOfLValue(CGF.MakeAddrLValue(V, DestTy)); } - + case CK_CPointerToObjCPointerCast: case CK_BlockPointerToObjCPointerCast: case CK_AnyPointerToBlockPointerCast: @@ -1124,6 +1124,10 @@ Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) { value = CGF.EmitARCRetainAutoreleasedReturnValue(value); return CGF.EmitObjCConsumeObject(E->getType(), value); } + case CK_ObjCExtendBlockObject: { + llvm::Value *value = CGF.EmitARCRetainScalarExpr(E); + return CGF.EmitObjCConsumeObject(E->getType(), value); + } case CK_FloatingRealToComplex: case CK_FloatingComplexCast: diff --git a/lib/CodeGen/CGObjC.cpp b/lib/CodeGen/CGObjC.cpp index 17ace4fc1a..037eed0430 100644 --- a/lib/CodeGen/CGObjC.cpp +++ b/lib/CodeGen/CGObjC.cpp @@ -1959,6 +1959,42 @@ static llvm::Value *emitARCRetainAfterCall(CodeGenFunction &CGF, } } +/// Determine whether it might be important to emit a separate +/// objc_retain_block on the result of the given expression, or +/// whether it's okay to just emit it in a +1 context. +static bool shouldEmitSeparateBlockRetain(const Expr *e) { + assert(e->getType()->isBlockPointerType()); + e = e->IgnoreParens(); + + // For future goodness, emit block expressions directly in +1 + // contexts if we can. + if (isa(e)) + return false; + + if (const CastExpr *cast = dyn_cast(e)) { + switch (cast->getCastKind()) { + // Emitting these operations in +1 contexts is goodness. + case CK_LValueToRValue: + case CK_ObjCReclaimReturnedObject: + case CK_ObjCConsumeObject: + case CK_ObjCProduceObject: + return false; + + // These operations preserve a block type. + case CK_NoOp: + case CK_BitCast: + return shouldEmitSeparateBlockRetain(cast->getSubExpr()); + + // These operations are known to be bad (or haven't been considered). + case CK_AnyPointerToBlockPointerCast: + default: + return true; + } + } + + return true; +} + static TryEmitResult tryEmitARCRetainScalarExpr(CodeGenFunction &CGF, const Expr *e) { // Look through cleanups. @@ -2015,6 +2051,40 @@ tryEmitARCRetainScalarExpr(CodeGenFunction &CGF, const Expr *e) { return TryEmitResult(result, true); } + // Block extends are net +0. Naively, we could just recurse on + // the subexpression, but actually we need to ensure that the + // value is copied as a block, so there's a little filter here. + case CK_ObjCExtendBlockObject: { + llvm::Value *result; // will be a +0 value + + // If we can't safely assume the sub-expression will produce a + // block-copied value, emit the sub-expression at +0. + if (shouldEmitSeparateBlockRetain(ce->getSubExpr())) { + result = CGF.EmitScalarExpr(ce->getSubExpr()); + + // Otherwise, try to emit the sub-expression at +1 recursively. + } else { + TryEmitResult subresult + = tryEmitARCRetainScalarExpr(CGF, ce->getSubExpr()); + result = subresult.getPointer(); + + // If that produced a retained value, just use that, + // possibly casting down. + if (subresult.getInt()) { + if (resultType) + result = CGF.Builder.CreateBitCast(result, resultType); + return TryEmitResult(result, true); + } + + // Otherwise it's +0. + } + + // Retain the object as a block, then cast down. + result = CGF.EmitARCRetainBlock(result); + if (resultType) result = CGF.Builder.CreateBitCast(result, resultType); + return TryEmitResult(result, true); + } + // For reclaims, emit the subexpression as a retained call and // skip the consumption. case CK_ObjCReclaimReturnedObject: { diff --git a/lib/Sema/SemaCXXCast.cpp b/lib/Sema/SemaCXXCast.cpp index 48f022063a..46f71be060 100644 --- a/lib/Sema/SemaCXXCast.cpp +++ b/lib/Sema/SemaCXXCast.cpp @@ -1633,13 +1633,7 @@ static TryCastResult TryReinterpretCast(Sema &Self, ExprResult &SrcExpr, if (IsLValueCast) { Kind = CK_LValueBitCast; } else if (DestType->isObjCObjectPointerType()) { - if (SrcType->isObjCObjectPointerType()) { - Kind = CK_BitCast; - } else if (SrcType->isBlockPointerType()) { - Kind = CK_BlockPointerToObjCPointerCast; - } else { - Kind = CK_CPointerToObjCPointerCast; - } + Kind = Self.PrepareCastToObjCObjectPointer(SrcExpr); } else if (DestType->isBlockPointerType()) { if (!SrcType->isBlockPointerType()) { Kind = CK_AnyPointerToBlockPointerCast; diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 75e03c131f..ab699e8c8a 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -3783,6 +3783,35 @@ Sema::ActOnInitList(SourceLocation LBraceLoc, MultiExprArg InitArgList, return Owned(E); } +/// Do an explicit extend of the given block pointer if we're in ARC. +static void maybeExtendBlockObject(Sema &S, ExprResult &E) { + assert(E.get()->getType()->isBlockPointerType()); + assert(E.get()->isRValue()); + + // Only do this in an r-value context. + if (!S.getLangOptions().ObjCAutoRefCount) return; + + E = ImplicitCastExpr::Create(S.Context, E.get()->getType(), + CK_ObjCExtendBlockObject, E.get(), + /*base path*/ 0, VK_RValue); + S.ExprNeedsCleanups = true; +} + +/// Prepare a conversion of the given expression to an ObjC object +/// pointer type. +CastKind Sema::PrepareCastToObjCObjectPointer(ExprResult &E) { + QualType type = E.get()->getType(); + if (type->isObjCObjectPointerType()) { + return CK_BitCast; + } else if (type->isBlockPointerType()) { + maybeExtendBlockObject(*this, E); + return CK_BlockPointerToObjCPointerCast; + } else { + assert(type->isPointerType()); + return CK_CPointerToObjCPointerCast; + } +} + /// Prepares for a scalar cast, performing all the necessary stages /// except the final cast and returning the kind required. static CastKind PrepareScalarCast(Sema &S, ExprResult &Src, QualType DestTy) { @@ -3812,8 +3841,10 @@ static CastKind PrepareScalarCast(Sema &S, ExprResult &Src, QualType DestTy) { return CK_BitCast; else if (SrcKind == Type::STK_CPointer) return CK_CPointerToObjCPointerCast; - else + else { + maybeExtendBlockObject(S, Src); return CK_BlockPointerToObjCPointerCast; + } case Type::STK_Bool: return CK_PointerToBoolean; case Type::STK_Integral: @@ -5458,6 +5489,7 @@ Sema::CheckAssignmentConstraints(QualType LHSType, ExprResult &RHS, // T^ -> A* if (RHSType->isBlockPointerType()) { + maybeExtendBlockObject(*this, RHS); Kind = CK_BlockPointerToObjCPointerCast; return Compatible; } diff --git a/lib/Sema/SemaExprCXX.cpp b/lib/Sema/SemaExprCXX.cpp index 1d903379c0..b8b722503a 100644 --- a/lib/Sema/SemaExprCXX.cpp +++ b/lib/Sema/SemaExprCXX.cpp @@ -2388,6 +2388,15 @@ Sema::PerformImplicitConversion(Expr *From, QualType ToType, CXXCastPath BasePath; if (CheckPointerConversion(From, ToType, Kind, BasePath, CStyle)) return ExprError(); + + // Make sure we extend blocks if necessary. + // FIXME: doing this here is really ugly. + if (Kind == CK_BlockPointerToObjCPointerCast) { + ExprResult E = From; + (void) PrepareCastToObjCObjectPointer(E); + From = E.take(); + } + From = ImpCastExprToType(From, ToType, Kind, VK_RValue, &BasePath, CCK) .take(); break; diff --git a/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/lib/StaticAnalyzer/Core/ExprEngineC.cpp index 1d7cd0d53f..db4dc2f2fe 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineC.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineC.cpp @@ -213,7 +213,8 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex, // since it understands retain/release semantics already. case CK_ObjCProduceObject: case CK_ObjCConsumeObject: - case CK_ObjCReclaimReturnedObject: // Fall-through. + case CK_ObjCReclaimReturnedObject: + case CK_ObjCExtendBlockObject: // Fall-through. // True no-ops. case CK_NoOp: case CK_FunctionToPointerDecay: { diff --git a/test/CodeGenObjC/arc.m b/test/CodeGenObjC/arc.m index 154918bb5b..7982fd6ebc 100644 --- a/test/CodeGenObjC/arc.m +++ b/test/CodeGenObjC/arc.m @@ -1855,3 +1855,40 @@ void test62(void) { // CHECK: call i8* @objc_getProperty // CHECK: call void @objc_setProperty +// rdar://problem/10088932 +void test64_helper(id); +void test64a(void) { + int x; + test64_helper(^{ (void) x; }); + + // CHECK: define void @test64a() + // CHECK: [[X:%.*]] = alloca i32, align 4 + // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]], align 8 + // CHECK: [[T0:%.*]] = bitcast [[BLOCK_T]]* [[BLOCK]] to void ()* + // 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: [[T4:%.*]] = bitcast void ()* [[T3]] to i8* + // CHECK-NEXT: call void @test64_helper(i8* [[T4]]) + // CHECK-NEXT: [[T5:%.*]] = bitcast void ()* [[T3]] to i8* + // CHECK-NEXT: call void @objc_release(i8* [[T5]]) + // CHECK-NEXT: ret void +} +void test64b(void) { + int x; + id b = ^{ (void) x; }; + + // CHECK: define void @test64b() + // CHECK: [[X:%.*]] = alloca i32, align 4 + // CHECK-NEXT: [[B:%.*]] = alloca i8*, align 8 + // CHECK-NEXT: [[BLOCK:%.*]] = alloca [[BLOCK_T:<{.*}>]], align 8 + // CHECK: [[T0:%.*]] = bitcast [[BLOCK_T]]* [[BLOCK]] to void ()* + // 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: [[T4:%.*]] = bitcast void ()* [[T3]] to i8* + // CHECK-NEXT: store i8* [[T4]], i8** [[B]], align 8 + // CHECK-NEXT: [[T5:%.*]] = load i8** [[B]] + // CHECK-NEXT: call void @objc_release(i8* [[T5]]) + // CHECK-NEXT: ret void +} -- 2.40.0