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

Author: rjmccall

include/clang/Sema/Sema.h

@@ -2101,6 +2101,8 @@ class Sema {
   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);

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<CallSyncExit>(NormalAndEHCleanup, syncExitFn,
-      SyncArg);
+  CGF.EHStack.pushCleanup<CallSyncExit>(NormalAndEHCleanup, syncExitFn, lock);
 
   // Emit the body of the statement.
   CGF.EmitStmt(S.getSynchBody());
-
-  // Pop the lock-release cleanup.
-  CGF.PopCleanupBlock();
 }

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:

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<PointerType>();
-    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<PointerType>();
+    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));
 }
 

lib/Sema/TreeTransform.h

@@ -1181,15 +1181,22 @@ class TreeTransform {
     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<Derived>::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());

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
+}