From: John McCall Date: Wed, 27 Jul 2011 21:50:02 +0000 (+0000) Subject: The lock operand to an @synchronized statement is also X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=07524039dce5c820f111a1b3f772b4261f004b4a;p=clang The lock operand to an @synchronized statement is also supposed to be a full-expression; make it so. In ARC, make sure we retain the lock for the entire protected block. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@136271 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index a14fba5300..f0d414b815 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -2101,6 +2101,8 @@ public: StmtResult BuildObjCAtThrowStmt(SourceLocation AtLoc, Expr *Throw); StmtResult ActOnObjCAtThrowStmt(SourceLocation AtLoc, Expr *Throw, Scope *CurScope); + ExprResult ActOnObjCAtSynchronizedOperand(SourceLocation atLoc, + Expr *operand); StmtResult ActOnObjCAtSynchronizedStmt(SourceLocation AtLoc, Expr *SynchExpr, Stmt *SynchBody); diff --git a/lib/CodeGen/CGObjCRuntime.cpp b/lib/CodeGen/CGObjCRuntime.cpp index c2978039b1..6d9b5b3374 100644 --- a/lib/CodeGen/CGObjCRuntime.cpp +++ b/lib/CodeGen/CGObjCRuntime.cpp @@ -288,21 +288,26 @@ void CGObjCRuntime::EmitAtSynchronizedStmt(CodeGenFunction &CGF, const ObjCAtSynchronizedStmt &S, llvm::Function *syncEnterFn, llvm::Function *syncExitFn) { - // Evaluate the lock operand. This should dominate the cleanup. - llvm::Value *SyncArg = - CGF.EmitScalarExpr(S.getSynchExpr()); + CodeGenFunction::RunCleanupsScope cleanups(CGF); + + // Evaluate the lock operand. This is guaranteed to dominate the + // ARC release and lock-release cleanups. + const Expr *lockExpr = S.getSynchExpr(); + llvm::Value *lock; + if (CGF.getLangOptions().ObjCAutoRefCount) { + lock = CGF.EmitARCRetainScalarExpr(lockExpr); + lock = CGF.EmitObjCConsumeObject(lockExpr->getType(), lock); + } else { + lock = CGF.EmitScalarExpr(lockExpr); + } + lock = CGF.Builder.CreateBitCast(lock, CGF.VoidPtrTy); // Acquire the lock. - SyncArg = CGF.Builder.CreateBitCast(SyncArg, syncEnterFn->getFunctionType()->getParamType(0)); - CGF.Builder.CreateCall(syncEnterFn, SyncArg); + CGF.Builder.CreateCall(syncEnterFn, lock)->setDoesNotThrow(); // Register an all-paths cleanup to release the lock. - CGF.EHStack.pushCleanup(NormalAndEHCleanup, syncExitFn, - SyncArg); + CGF.EHStack.pushCleanup(NormalAndEHCleanup, syncExitFn, lock); // Emit the body of the statement. CGF.EmitStmt(S.getSynchBody()); - - // Pop the lock-release cleanup. - CGF.PopCleanupBlock(); } diff --git a/lib/Parse/ParseObjc.cpp b/lib/Parse/ParseObjc.cpp index ff9e20504b..c8b2a09d5e 100644 --- a/lib/Parse/ParseObjc.cpp +++ b/lib/Parse/ParseObjc.cpp @@ -1560,31 +1560,46 @@ Parser::ParseObjCSynchronizedStmt(SourceLocation atLoc) { Diag(Tok, diag::err_expected_lparen_after) << "@synchronized"; return StmtError(); } + + // The operand is surrounded with parentheses. ConsumeParen(); // '(' - ExprResult Res(ParseExpression()); - if (Res.isInvalid()) { - SkipUntil(tok::semi); - return StmtError(); - } - if (Tok.isNot(tok::r_paren)) { - Diag(Tok, diag::err_expected_lbrace); - return StmtError(); + ExprResult operand(ParseExpression()); + + if (Tok.is(tok::r_paren)) { + ConsumeParen(); // ')' + } else { + if (!operand.isInvalid()) + Diag(Tok, diag::err_expected_rparen); + + // Skip forward until we see a left brace, but don't consume it. + SkipUntil(tok::l_brace, true, true); } - ConsumeParen(); // ')' + + // Require a compound statement. if (Tok.isNot(tok::l_brace)) { - Diag(Tok, diag::err_expected_lbrace); + if (!operand.isInvalid()) + Diag(Tok, diag::err_expected_lbrace); return StmtError(); } - // Enter a scope to hold everything within the compound stmt. Compound - // statements can always hold declarations. - ParseScope BodyScope(this, Scope::DeclScope); - StmtResult SynchBody(ParseCompoundStatementBody()); + // Check the @synchronized operand now. + if (!operand.isInvalid()) + operand = Actions.ActOnObjCAtSynchronizedOperand(atLoc, operand.take()); - BodyScope.Exit(); - if (SynchBody.isInvalid()) - SynchBody = Actions.ActOnNullStmt(Tok.getLocation()); - return Actions.ActOnObjCAtSynchronizedStmt(atLoc, Res.take(), SynchBody.take()); + // Parse the compound statement within a new scope. + ParseScope bodyScope(this, Scope::DeclScope); + StmtResult body(ParseCompoundStatementBody()); + bodyScope.Exit(); + + // If there was a semantic or parse error earlier with the + // operand, fail now. + if (operand.isInvalid()) + return StmtError(); + + if (body.isInvalid()) + body = Actions.ActOnNullStmt(Tok.getLocation()); + + return Actions.ActOnObjCAtSynchronizedStmt(atLoc, operand.get(), body.get()); } /// objc-try-catch-statement: diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 56161ed9b4..5825d6b9b6 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -2232,25 +2232,32 @@ Sema::ActOnObjCAtThrowStmt(SourceLocation AtLoc, Expr *Throw, return BuildObjCAtThrowStmt(AtLoc, Throw); } -StmtResult -Sema::ActOnObjCAtSynchronizedStmt(SourceLocation AtLoc, Expr *SyncExpr, - Stmt *SyncBody) { - getCurFunction()->setHasBranchProtectedScope(); - - ExprResult Result = DefaultLvalueConversion(SyncExpr); - if (Result.isInvalid()) - return StmtError(); +ExprResult +Sema::ActOnObjCAtSynchronizedOperand(SourceLocation atLoc, Expr *operand) { + ExprResult result = DefaultLvalueConversion(operand); + if (result.isInvalid()) + return ExprError(); + operand = result.take(); - SyncExpr = Result.take(); // Make sure the expression type is an ObjC pointer or "void *". - if (!SyncExpr->getType()->isDependentType() && - !SyncExpr->getType()->isObjCObjectPointerType()) { - const PointerType *PT = SyncExpr->getType()->getAs(); - if (!PT || !PT->getPointeeType()->isVoidType()) - return StmtError(Diag(AtLoc, diag::error_objc_synchronized_expects_object) - << SyncExpr->getType() << SyncExpr->getSourceRange()); + QualType type = operand->getType(); + if (!type->isDependentType() && + !type->isObjCObjectPointerType()) { + const PointerType *pointerType = type->getAs(); + if (!pointerType || !pointerType->getPointeeType()->isVoidType()) + return Diag(atLoc, diag::error_objc_synchronized_expects_object) + << type << operand->getSourceRange(); } + // The operand to @synchronized is a full-expression. + return MaybeCreateExprWithCleanups(operand); +} + +StmtResult +Sema::ActOnObjCAtSynchronizedStmt(SourceLocation AtLoc, Expr *SyncExpr, + Stmt *SyncBody) { + // We can't jump into or indirect-jump out of a @synchronized block. + getCurFunction()->setHasBranchProtectedScope(); return Owned(new (Context) ObjCAtSynchronizedStmt(AtLoc, SyncExpr, SyncBody)); } diff --git a/lib/Sema/TreeTransform.h b/lib/Sema/TreeTransform.h index 373339a2cb..12eba29799 100644 --- a/lib/Sema/TreeTransform.h +++ b/lib/Sema/TreeTransform.h @@ -1181,15 +1181,22 @@ public: return getSema().BuildObjCAtThrowStmt(AtLoc, Operand); } + /// \brief Rebuild the operand to an Objective-C @synchronized statement. + /// + /// By default, performs semantic analysis to build the new statement. + /// Subclasses may override this routine to provide different behavior. + ExprResult RebuildObjCAtSynchronizedOperand(SourceLocation atLoc, + Expr *object) { + return getSema().ActOnObjCAtSynchronizedOperand(atLoc, object); + } + /// \brief Build a new Objective-C @synchronized statement. /// /// By default, performs semantic analysis to build the new statement. /// Subclasses may override this routine to provide different behavior. StmtResult RebuildObjCAtSynchronizedStmt(SourceLocation AtLoc, - Expr *Object, - Stmt *Body) { - return getSema().ActOnObjCAtSynchronizedStmt(AtLoc, Object, - Body); + Expr *Object, Stmt *Body) { + return getSema().ActOnObjCAtSynchronizedStmt(AtLoc, Object, Body); } /// \brief Build a new Objective-C @autoreleasepool statement. @@ -5479,6 +5486,11 @@ TreeTransform::TransformObjCAtSynchronizedStmt( ExprResult Object = getDerived().TransformExpr(S->getSynchExpr()); if (Object.isInvalid()) return StmtError(); + Object = + getDerived().RebuildObjCAtSynchronizedOperand(S->getAtSynchronizedLoc(), + Object.get()); + if (Object.isInvalid()) + return StmtError(); // Transform the body. StmtResult Body = getDerived().TransformStmt(S->getSynchBody()); diff --git a/test/CodeGenObjC/arc.m b/test/CodeGenObjC/arc.m index 918cbcbb3b..3d0abc5c30 100644 --- a/test/CodeGenObjC/arc.m +++ b/test/CodeGenObjC/arc.m @@ -1666,3 +1666,21 @@ void test58b(void) { __attribute__((objc_precise_lifetime)) Test58 *ptr = test58_helper(); char *c = [ptr interior]; } + +// rdar://problem/9842343 +void test59(void) { + extern id test59_getlock(void); + extern void test59_body(void); + @synchronized (test59_getlock()) { + test59_body(); + } + + // CHECK: define void @test59() + // CHECK: [[T0:%.*]] = call i8* @test59_getlock() + // CHECK-NEXT: [[T1:%.*]] = call i8* @objc_retainAutoreleasedReturnValue(i8* [[T0]]) + // CHECK-NEXT: call void @objc_sync_enter(i8* [[T1]]) + // CHECK-NEXT: call void @test59_body() + // CHECK-NEXT: call void @objc_sync_exit(i8* [[T1]]) + // CHECK-NEXT: call void @objc_release(i8* [[T1]]) + // CHECK-NEXT: ret void +}