Thread Safety: Added basic argument parsing for all new attributes.

This patch special cases the parser for thread safety attributes so that all
attribute arguments are put in the argument list (instead of a special
parameter) since arguments may not otherwise resolve correctly without two-token
lookahead.

This patch also adds checks to make sure that attribute arguments are
lockable objects.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@137130 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 738 insertions, 163 deletions

diff --git a/docs/LanguageExtensions.html b/docs/LanguageExtensions.html
index 0bb64e8dd2..5003d9cad8 100644
--- a/docs/LanguageExtensions.html
+++ b/docs/LanguageExtensions.html
@@ -1164,12 +1164,12 @@ specify that the pointer must be dereferenced while holding some lock.</p>
 <h4 id="ts_guardedby">guarded_by(l)</h4>
 
 <p>Use <tt>__attribute__((guarded_by(l)))</tt> on a variable declaration to 
-specify that the variable must be accessed while holding lock l.</p>
+specify that the variable must be accessed while holding lock <tt>l</tt>.</p>
 
 <h4 id="ts_ptguardedby">pt_guarded_by(l)</h4>
 
 <p>Use <tt>__attribute__((pt_guarded_by(l)))</tt> on a pointer declaration to 
-specify that the pointer must be dereferenced while holding lock l.</p>
+specify that the pointer must be dereferenced while holding lock <tt>l</tt>.</p>
 
 <h4 id="ts_acquiredbefore">acquired_before(...)</h4>
 
@@ -1189,67 +1189,62 @@ least one argument.</p>
 
 <p>Use <tt>__attribute__((exclusive_lock_function(...)))</tt> on a function 
 declaration to specify that the function acquires all listed locks 
-exclusively. This attribute takes zero or more 
-arguments: either of lockable type or integers indexing into 
-function parameters of lockable type. If no arguments are given, the acquired 
-lock is implicitly <tt>this</tt> of the enclosing object.</p>
+exclusively. This attribute takes zero or more arguments: either of lockable 
+type or integers indexing into function parameters of lockable type. If no 
+arguments are given, the acquired lock is implicitly <tt>this</tt> of the 
+enclosing object.</p>
 
 <h4 id="ts_slf">shared_lock_function(...)</h4>
 
 <p>Use <tt>__attribute__((shared_lock_function(...)))</tt> on a function 
 declaration to specify that the function acquires all listed locks, although
- the locks may be shared (e.g. read locks). 
-This attribute takes zero or more 
-arguments: either of lockable type or integers indexing into 
-function parameters of lockable type. If no arguments are given, the acquired 
-lock is implicitly <tt>this</tt> of the enclosing object.</p>
+ the locks may be shared (e.g. read locks). This attribute takes zero or more 
+arguments: either of lockable type or integers indexing into function 
+parameters of lockable type. If no arguments are given, the acquired lock is 
+implicitly <tt>this</tt> of the enclosing object.</p>
 
 <h4 id="ts_etf">exclusive_trylock_function(...)</h4>
 
 <p>Use <tt>__attribute__((exclusive_lock_function(...)))</tt> on a function 
 declaration to specify that the function will try (without blocking) to acquire
-all listed locks exclusively. This attribute takes one or more 
-arguments. The first argument is an integer or boolean value specifying the 
-return value of a successful lock acquisition. The remaining arugments are 
-either of lockable type or integers indexing into 
-function parameters of lockable type. If only one argument is given, the 
-acquired lock is implicitly <tt>this</tt> of the enclosing object.</p>
+all listed locks exclusively. This attribute takes one or more arguments. The 
+first argument is an integer or boolean value specifying the return value of a 
+successful lock acquisition. The remaining arugments are either of lockable type 
+or integers indexing into function parameters of lockable type. If only one 
+argument is given, the acquired lock is implicitly <tt>this</tt> of the 
+enclosing object.</p>
 
 <h4 id="ts_stf">shared_trylock_function(...)</h4>
 
 <p>Use <tt>__attribute__((shared_lock_function(...)))</tt> on a function 
 declaration to specify that the function will try (without blocking) to acquire
-all listed locks, although
- the locks may be shared (e.g. read locks). 
-This attribute takes one or more 
-arguments. The first argument is an integer or boolean value specifying the 
-return value of a successful lock acquisition. The remaining arugments are 
-either of lockable type or integers indexing into 
+all listed locks, although the locks may be shared (e.g. read locks). This 
+attribute takes one or more arguments. The first argument is an integer or 
+boolean value specifying the return value of a successful lock acquisition. The 
+remaining arugments are either of lockable type or integers indexing into 
 function parameters of lockable type. If only one argument is given, the 
 acquired lock is implicitly <tt>this</tt> of the enclosing object.</p>
 
 <h4 id="ts_uf">unlock_function(...)</h4>
 
 <p>Use <tt>__attribute__((unlock_function(...)))</tt> on a function 
-declaration to specify that the function release all listed locks.
- This attribute takes zero or more 
-arguments: either of lockable type or integers indexing into 
-function parameters of lockable type. If no arguments are given, the acquired 
-lock is implicitly <tt>this</tt> of the enclosing object.</p>
+declaration to specify that the function release all listed locks. This 
+attribute takes zero or more arguments: either of lockable type or integers 
+indexing into function parameters of lockable type. If no arguments are given, 
+the acquired lock is implicitly <tt>this</tt> of the enclosing object.</p>
 
 <h4 id="ts_lr">lock_returned(l)</h4>
 
 <p>Use <tt>__attribute__((lock_returned(l)))</tt> on a function 
-declaration to specify that the function returns lock l (l must be of lockable 
-type). This annotation is used
-to aid in resolving lock expressions.</p>
+declaration to specify that the function returns lock <tt>l</tt> (<tt>l</tt> 
+must be of lockable type). This annotation is used to aid in resolving lock 
+expressions.</p>
 
 <h4 id="ts_le">locks_excluded(...)</h4>
 
 <p>Use <tt>__attribute__((locks_excluded(...)))</tt> on a function declaration 
-to specify that the function must not be called with the listed locks. 
-Arguments must be lockable type, and there must be at 
-least one argument.</p>
+to specify that the function must not be called with the listed locks. Arguments 
+must be lockable type, and there must be at least one argument.</p>
 
 <h4 id="ts_elr">exclusive_locks_required(...)</h4>
 
diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index a300764733..7c5c217ce3 100644
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -274,6 +274,9 @@ def Most : DiagGroup<"most", [
     OverloadedVirtual
  ]>;
 
+// Thread Safety warnings 
+def : DiagGroup<"thread-safety">;
+
 // -Wall is -Wmost -Wparentheses
 def : DiagGroup<"all", [Most, Parentheses]>;
 
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 38db5b3ec6..37409d0900 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1142,6 +1142,10 @@ def err_attribute_bad_neon_vector_size : Error<
   "Neon vector size must be 64 or 128 bits">;
 def err_attribute_argument_not_int : Error<
   "'%0' attribute requires integer constant">;
+def err_attribute_argument_not_class : Error<
+  "%0 attribute requires arguments that are class type or point to class type">;
+def err_attribute_first_argument_not_int_or_bool : Error<
+  "%0 attribute first argument must be of int or bool type">;
 def err_attribute_argument_outof_range : Error<
   "init_priority attribute requires integer constant between "
   "101 and 65535 inclusive">;
@@ -1300,6 +1304,21 @@ def warn_availability_version_ordering : Warning<
   "feature cannot be %select{introduced|deprecated|obsoleted}0 in %1 version "
   "%2 before it was %select{introduced|deprecated|obsoleted}3 in version %4; "
   "attribute ignored">;
+  
+// Thread Safety Attributes
+// Errors when parsing the attributes
+def err_attribute_argument_out_of_range : Error<
+  "%0 attribute parameter %1 is out of bounds: "
+  "%plural{0:no parameters to index into|"
+  "1:can only be 1, since there is one parameter|"
+  ":must be between 1 and %2}2">;
+def err_attribute_argument_not_lockable : Error<
+  "%0 attribute requires arguments whose type is annotated "
+  "with 'lockable' attribute">;
+def err_attribute_decl_not_lockable : Error<
+  "%0 attribute can only be applied in a context annotated "
+  "with 'lockable' attribute">;
+
 
 def warn_impcast_vector_scalar : Warning<
   "implicit conversion turns vector to scalar: %0 to %1">,
diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h
index af3f40ec40..fc5d11cb6a 100644
--- a/include/clang/Parse/Parser.h
+++ b/include/clang/Parse/Parser.h
@@ -1691,6 +1691,13 @@ bool ParseAsmOperandsOpt(SmallVectorImpl<IdentifierInfo *> &Names,
                                   ParsedAttributes &attrs,
                                   SourceLocation *endLoc);
 
+  bool IsThreadSafetyAttribute(llvm::StringRef AttrName);
+  void ParseThreadSafetyAttribute(IdentifierInfo &AttrName,
+                                  SourceLocation AttrNameLoc,
+                                  ParsedAttributes &Attrs,
+                                  SourceLocation *EndLoc);
+
+
   void ParseTypeofSpecifier(DeclSpec &DS);
   void ParseDecltypeSpecifier(DeclSpec &DS);
   void ParseUnderlyingTypeSpecifier(DeclSpec &DS);
diff --git a/lib/Parse/ParseDecl.cpp b/lib/Parse/ParseDecl.cpp
index 1e318fe66e..2a95ecc7c7 100644
--- a/lib/Parse/ParseDecl.cpp
+++ b/lib/Parse/ParseDecl.cpp
@@ -19,6 +19,7 @@
 #include "clang/Sema/PrettyDeclStackTrace.h"
 #include "RAIIObjectsForParser.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringSwitch.h"
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
@@ -121,6 +122,10 @@ void Parser::ParseGNUAttributes(ParsedAttributes &attrs,
       // Availability attributes have their own grammar.
       if (AttrName->isStr("availability"))
         ParseAvailabilityAttribute(*AttrName, AttrNameLoc, attrs, endLoc);
+      // Thread safety attributes fit into the FIXME case above, so we
+      // just parse the arguments as a list of expressions
+      else if (IsThreadSafetyAttribute(AttrName->getName()))
+        ParseThreadSafetyAttribute(*AttrName, AttrNameLoc, attrs, endLoc);
       // check if we have a "parameterized" attribute
       else if (Tok.is(tok::l_paren)) {
         ConsumeParen(); // ignore the left paren loc for now
@@ -650,6 +655,81 @@ void Parser::ParseAvailabilityAttribute(IdentifierInfo &Availability,
                UnavailableLoc, false, false);
 }
 
+/// \brief Wrapper around a case statement checking if AttrName is
+/// one of the thread safety attributes
+bool Parser::IsThreadSafetyAttribute(llvm::StringRef AttrName){
+  return llvm::StringSwitch<bool>(AttrName)
+      .Case("guarded_by", true)
+      .Case("guarded_var", true)
+      .Case("pt_guarded_by", true)
+      .Case("pt_guarded_var", true)
+      .Case("lockable", true)
+      .Case("scoped_lockable", true)
+      .Case("no_thread_safety_analysis", true)
+      .Case("acquired_after", true)
+      .Case("acquired_before", true)
+      .Case("exclusive_lock_function", true)
+      .Case("shared_lock_function", true)
+      .Case("exclusive_trylock_function", true)
+      .Case("shared_trylock_function", true)
+      .Case("unlock_function", true)
+      .Case("lock_returned", true)
+      .Case("locks_excluded", true)
+      .Case("exclusive_locks_required", true)
+      .Case("shared_locks_required", true)
+      .Default(false);
+}
+
+/// \brief Parse the contents of thread safety attributes. These
+/// should always be parsed as an expression list.
+///
+/// We need to special case the parsing due to the fact that if the first token
+/// of the first argument is an identifier, the main parse loop will store
+/// that token as a "parameter" and the rest of
+/// the arguments will be added to a list of "arguments". However,
+/// subsequent tokens in the first argument are lost. We instead parse each
+/// argument as an expression and add all arguments to the list of "arguments".
+/// In future, we will take advantage of this special case to also
+/// deal with some argument scoping issues here (for example, referring to a
+/// function parameter in the attribute on that function).
+void Parser::ParseThreadSafetyAttribute(IdentifierInfo &AttrName,
+                                        SourceLocation AttrNameLoc,
+                                        ParsedAttributes &Attrs,
+                                        SourceLocation *EndLoc) {
+
+  if (Tok.is(tok::l_paren)) {
+    SourceLocation LeftParenLoc = Tok.getLocation();
+    ConsumeParen(); // ignore the left paren loc for now
+
+    ExprVector ArgExprs(Actions);
+    bool ArgExprsOk = true;
+
+    // now parse the list of expressions
+    while (1) {
+      ExprResult ArgExpr(ParseAssignmentExpression());
+      if (ArgExpr.isInvalid()) {
+        ArgExprsOk = false;
+        MatchRHSPunctuation(tok::r_paren, LeftParenLoc);
+        break;
+      } else {
+        ArgExprs.push_back(ArgExpr.release());
+      }
+      if (Tok.isNot(tok::comma))
+        break;
+      ConsumeToken(); // Eat the comma, move to the next argument
+    }
+    // Match the ')'.
+    if (ArgExprsOk && Tok.is(tok::r_paren)) {
+      ConsumeParen(); // ignore the right paren loc for now
+      Attrs.addNew(&AttrName, AttrNameLoc, 0, AttrNameLoc, 0, SourceLocation(),
+                   ArgExprs.take(), ArgExprs.size());
+    }
+  } else {
+    Attrs.addNew(&AttrName, AttrNameLoc, 0, AttrNameLoc,
+                 0, SourceLocation(), 0, 0);
+  }
+}
+
 void Parser::DiagnoseProhibitedAttributes(ParsedAttributesWithRange &attrs) {
   Diag(attrs.Range.getBegin(), diag::err_attributes_not_allowed)
     << attrs.Range;
diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
index 9fbd73f4b6..0644103b44 100644
--- a/lib/Sema/SemaDeclAttr.cpp
+++ b/lib/Sema/SemaDeclAttr.cpp
@@ -15,6 +15,7 @@
 #include "TargetAttributesSema.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/Expr.h"
 #include "clang/Basic/SourceManager.h"
@@ -27,7 +28,7 @@ using namespace sema;
 
 /// These constants match the enumerated choices of
 /// warn_attribute_wrong_decl_type and err_attribute_wrong_decl_type.
-enum AttributeDeclType {
+enum AttributeDeclKind {
   ExpectedFunction,
   ExpectedUnion,
   ExpectedVariableOrFunction,
@@ -195,6 +196,8 @@ static inline bool isCFStringType(QualType T, ASTContext &Ctx) {
   return RD->getIdentifier() == &Ctx.Idents.get("__CFString");
 }
 
+/// \brief Check if the attribute has exactly as many args as Num. May
+/// output an error.
 static bool checkAttributeNumArgs(Sema &S, const AttributeList &Attr,
                                   unsigned int Num) {
   if (Attr.getNumArgs() != Num) {
@@ -205,31 +208,16 @@ static bool checkAttributeNumArgs(Sema &S, const AttributeList &Attr,
   return true;
 }
 
-///
-/// \brief Check the total number of argumenation, whether parsed by clang
-/// as arguments or parameters. Outputs a warning.
-/// \return false if the number of argumenation units does not match expectation
-///
-static bool checkAttributeNumArgsPlusParams(Sema &S, const AttributeList &Attr,
-                                             unsigned int Num,
-                                             bool moreok = false) {
-  unsigned int numArgsPlusParams = 0;
-
-  if (Attr.getParameterName())
-    numArgsPlusParams++;
 
-  numArgsPlusParams += Attr.getNumArgs();
-
-  if (moreok && numArgsPlusParams < Num) {
+/// \brief Check if the attribute has at least as many args as Num. May
+/// output an error.
+static bool checkAttributeAtLeastNumArgs(Sema &S, const AttributeList &Attr,
+                                  unsigned int Num) {
+  if (Attr.getNumArgs() < Num) {
     S.Diag(Attr.getLoc(), diag::err_attribute_too_few_arguments) << Num;
     return false;
   }
 
-  if (!moreok && numArgsPlusParams != Num) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_wrong_number_arguments) << Num;
-    return false;
-  }
-
   return true;
 }
 
@@ -246,6 +234,12 @@ static bool mayBeSharedVariable(const Decl *D) {
   return false;
 }
 
+/// \brief Check if the passed-in expression is of type int or bool.
+static bool isIntOrBool(Expr *Exp) {
+  QualType QT = Exp->getType();
+  return QT->isBooleanType() || QT->isIntegerType();
+}
+
 ///
 /// \brief Check if passed in Decl is a pointer type.
 /// Note that this function may produce an error message.
@@ -265,6 +259,71 @@ static bool checkIsPointer(Sema &S, const Decl *D, const AttributeList &Attr) {
   return false;
 }
 
+/// \brief Checks that the passed in QualType either is of RecordType or points
+/// to RecordType. Returns the relevant RecordType, null if it does not exit.
+const RecordType *getRecordType(QualType QT) {
+    const RecordType *RT = QT->getAs<RecordType>();
+    // now check if we point to record type
+    if(!RT && QT->isPointerType()){
+        QualType PT = QT->getAs<PointerType>()->getPointeeType();
+        RT = PT->getAs<RecordType>();
+    }
+    return RT;
+}
+
+/// \brief Thread Safety Analysis: Checks that all attribute arguments, starting
+/// from Sidx, resolve to a lockable object. May flag an error.
+static bool checkAttrArgsAreLockableObjs(Sema & S, Decl *D,
+                                         const AttributeList & Attr,
+                                         int Sidx = 0,
+                                         bool ParamIdxOk = false) {
+  for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) {
+    Expr *ArgExp = Attr.getArg(Idx);
+    if (ArgExp->isTypeDependent())
+      continue;
+
+    QualType Arg_QT = ArgExp->getType();
+
+    // Get record type.
+    // first see if we can just cast to record type, or point to record type
+    const RecordType *RT = getRecordType(Arg_QT);
+
+    // now check if we idx into a record type function param
+    if (!RT && ParamIdxOk) {
+      FunctionDecl *FD = dyn_cast <FunctionDecl>(D);
+      IntegerLiteral *IL = dyn_cast<IntegerLiteral>(ArgExp);
+      if(FD && IL) {
+        unsigned int NumParams = FD->getNumParams();
+        llvm::APInt ArgValue = IL->getValue();
+        uint64_t ParamIdx_from1 = ArgValue.getZExtValue();
+        uint64_t ParamIdx_from0 = ParamIdx_from1 - 1;
+        if(!ArgValue.isStrictlyPositive() || ParamIdx_from1 > NumParams) {
+          S.Diag(Attr.getLoc(), diag::err_attribute_argument_out_of_range)
+            << Attr.getName() << Idx + 1 << NumParams;
+          return false;
+        }
+        Arg_QT = FD->getParamDecl(ParamIdx_from0)->getType();
+        RT = getRecordType(Arg_QT);
+      }
+    }
+
+    //  Flag error if could not get record type for this argument
+    if (!RT) {
+      S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_class)
+        << Attr.getName();
+      return false;
+    }
+
+    // Flag error if the type is not lockable
+    if (!RT->getDecl()->getAttr<LockableAttr>()) {
+      S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_lockable)
+        << Attr.getName();
+      return false;
+    }
+  }
+  return true;
+}
+
 //===----------------------------------------------------------------------===//
 // Attribute Implementations
 //===----------------------------------------------------------------------===//
@@ -283,7 +342,7 @@ static void handleGuardedVarAttr(Sema &S, Decl *D, const AttributeList &Attr,
   // D must be either a member field or global (potentially shared) variable.
   if (!mayBeSharedVariable(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
-      << Attr.getName() << 15; /*fields and global vars*/;
+      << Attr.getName() << ExpectedFieldOrGlobalVar;
     return;
   }
 
@@ -297,22 +356,26 @@ static void handleGuardedVarAttr(Sema &S, Decl *D, const AttributeList &Attr,
 }
 
 static void handleGuardedByAttr(Sema &S, Decl *D, const AttributeList &Attr,
-                                 bool pointer = false) {
+                                bool pointer = false) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1))
+  if (!checkAttributeNumArgs(S, Attr, 1))
     return;
 
   // D must be either a member field or global (potentially shared) variable.
   if (!mayBeSharedVariable(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
-      << Attr.getName() << 15; /*fields and global vars*/;
+      << Attr.getName() << ExpectedFieldOrGlobalVar;
     return;
   }
 
   if (pointer && !checkIsPointer(S, D, Attr))
     return;
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr))
+    return;
+
   if (pointer)
     D->addAttr(::new (S.Context) PtGuardedByAttr(Attr.getLoc(), S.Context));
   else
@@ -346,7 +409,7 @@ static void handleNoThreadSafetyAttr(Sema &S, Decl *D,
   if (!checkAttributeNumArgs(S, Attr, 0))
     return;
 
-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
@@ -360,16 +423,32 @@ static void handleAcquireOrderAttr(Sema &S, Decl *D, const AttributeList &Attr,
                                    bool before) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1, /*moreok=*/true))
+  if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 
   // D must be either a member field or global (potentially shared) variable.
-  if (!mayBeSharedVariable(D)) {
+  ValueDecl *VD = dyn_cast<ValueDecl>(D);
+  if (!VD || !mayBeSharedVariable(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
-      << Attr.getName() << 15; /*fields and global vars*/;
+      << Attr.getName() << ExpectedFieldOrGlobalVar;
     return;
   }
 
+  // Check that this attribute only applies to lockable types
+  QualType QT = VD->getType();
+  if (!QT->isDependentType()) {
+    const RecordType *RT = getRecordType(QT);
+    if (!RT || !RT->getDecl()->getAttr<LockableAttr>()) {
+      S.Diag(Attr.getLoc(), diag::err_attribute_decl_not_lockable)
+              << Attr.getName();
+      return;
+    }
+  }
+
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr))
+    return;
+
   if (before)
     D->addAttr(::new (S.Context) AcquiredBeforeAttr(Attr.getLoc(), S.Context));
   else
@@ -377,17 +456,22 @@ static void handleAcquireOrderAttr(Sema &S, Decl *D, const AttributeList &Attr,
 }
 
 static void handleLockFunAttr(Sema &S, Decl *D, const AttributeList &Attr,
-                                   bool exclusive = false) {
+                              bool exclusive = false) {
   assert(!Attr.isInvalid());
 
   // zero or more arguments ok
 
-  if (!isFunction(D)) {
+  // check that the attribute is applied to a function
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr, 0, /*ParamIdxOk=*/true))
+    return;
+
   if (exclusive)
     D->addAttr(::new (S.Context) ExclusiveLockFunctionAttr(Attr.getLoc(),
                                                            S.Context));
@@ -397,93 +481,118 @@ static void handleLockFunAttr(Sema &S, Decl *D, const AttributeList &Attr,
 }
 
 static void handleTrylockFunAttr(Sema &S, Decl *D, const AttributeList &Attr,
-                                      bool exclusive = false) {
+                                 bool exclusive = false) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1, /*moreok=*/true))
+  if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 
-  if (!isFunction(D)) {
+
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  if (!isIntOrBool(Attr.getArg(0))) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_first_argument_not_int_or_bool)
+        << Attr.getName();
+    return;
+  }
+
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr, 1))
+    return;
+
   if (exclusive)
     D->addAttr(::new (S.Context) ExclusiveTrylockFunctionAttr(Attr.getLoc(),
-                                                           S.Context));
+                                                              S.Context));
   else
     D->addAttr(::new (S.Context) SharedTrylockFunctionAttr(Attr.getLoc(),
-                                                        S.Context));
-
+                                                           S.Context));
 }
 
 static void handleLocksRequiredAttr(Sema &S, Decl *D, const AttributeList &Attr,
-                                   bool exclusive = false) {
+                                    bool exclusive = false) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1, /*moreok=*/true))
+  if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 
-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr))
+    return;
+
   if (exclusive)
     D->addAttr(::new (S.Context) ExclusiveLocksRequiredAttr(Attr.getLoc(),
-                                                           S.Context));
+                                                            S.Context));
   else
     D->addAttr(::new (S.Context) SharedLocksRequiredAttr(Attr.getLoc(),
-                                                        S.Context));
+                                                         S.Context));
 }
 
-
 static void handleUnlockFunAttr(Sema &S, Decl *D,
-                                     const AttributeList &Attr) {
+                                const AttributeList &Attr) {
   assert(!Attr.isInvalid());
 
   // zero or more arguments ok
 
-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr, 0, /*ParamIdxOk=*/true))
+    return;
+
   D->addAttr(::new (S.Context) UnlockFunctionAttr(Attr.getLoc(), S.Context));
 }
 
 static void handleLockReturnedAttr(Sema &S, Decl *D,
-                                     const AttributeList &Attr) {
+                                   const AttributeList &Attr) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1))
+  if (!checkAttributeNumArgs(S, Attr, 1))
     return;
 
-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr))
+    return;
+
   D->addAttr(::new (S.Context) LockReturnedAttr(Attr.getLoc(), S.Context));
 }
 
 static void handleLocksExcludedAttr(Sema &S, Decl *D,
-                                     const AttributeList &Attr) {
+                                    const AttributeList &Attr) {
   assert(!Attr.isInvalid());
 
-  if (!checkAttributeNumArgsPlusParams(S, Attr, 1, /*moreok=*/true))
+  if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
     return;
 
-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D) && !isa<FunctionTemplateDecl>(D)) {
     S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
       << Attr.getName() << ExpectedFunctionOrMethod;
     return;
   }
 
+  // check that all arguments are lockable objects
+  if (!checkAttrArgsAreLockableObjs(S, D, Attr))
+    return;
+
   D->addAttr(::new (S.Context) LocksExcludedAttr(Attr.getLoc(), S.Context));
 }
 
diff --git a/test/SemaCXX/warn-thread-safety.cpp b/test/SemaCXX/warn-thread-safety.cpp
index 4958d2ed0f..c5523e2a13 100644
--- a/test/SemaCXX/warn-thread-safety.cpp
+++ b/test/SemaCXX/warn-thread-safety.cpp
@@ -1,19 +1,48 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety %s
 
 
-/**
- *  Helper fields
- */
+//-----------------------------------------//
+//  Helper fields
+//-----------------------------------------//
 
 class __attribute__((lockable)) Mu {
 };
 
+class UnlockableMu{
+};
+
+class MuWrapper {
+  public:
+  Mu mu;
+  Mu getMu() {
+    return mu;
+  }
+  Mu * getMuPointer() {
+    return &mu;
+  }
+};
+
+
+class MuDoubleWrapper {
+  public:
+  MuWrapper* muWrapper;
+  MuWrapper* getWrapper() {
+    return muWrapper;
+  }
+};
+
 Mu mu1;
+UnlockableMu umu;
 Mu mu2;
+MuWrapper muWrapper;
+MuDoubleWrapper muDoubleWrapper;
+Mu* muPointer;
+Mu ** muDoublePointer = & muPointer;
+Mu& muRef = mu1;
 
-/***********************************
- *  No Thread Safety Analysis (noanal)
- ***********************************/
+//-----------------------------------------//
+//   No Thread Safety Analysis (noanal)    //
+//-----------------------------------------//
 
 // FIXME: Right now we cannot parse attributes put on function definitions
 // We would like to patch this at some point.
@@ -53,9 +82,9 @@ void noanal_fun_params(int lvar __attribute__((no_thread_safety_analysis))); //
   expected-warning {{'no_thread_safety_analysis' attribute only applies to functions and methods}}
 
 
-/***********************************
- *  Guarded Var Attribute (gv)
- ***********************************/
+//-----------------------------------------//
+//  Guarded Var Attribute (gv)
+//-----------------------------------------//
 
 #if !__has_attribute(guarded_var)
 #error "Should support guarded_var attribute"
@@ -89,9 +118,9 @@ int gv_testfn(int y){
   return x;
 }
 
-/***********************************
- *  Pt Guarded Var Attribute (pgv)
- ***********************************/
+//-----------------------------------------//
+//   Pt Guarded Var Attribute (pgv)
+//-----------------------------------------//
 
 //FIXME: add support for boost::scoped_ptr<int> fancyptr  and references
 
@@ -133,9 +162,9 @@ void pgv_testfn(int y){
   delete x;
 }
 
-/***********************************
- *  Lockable Attribute (l)
- ***********************************/
+//-----------------------------------------//
+//  Lockable Attribute (l)
+//-----------------------------------------//
 
 //FIXME: In future we may want to add support for structs, ObjC classes, etc.
 
@@ -175,9 +204,9 @@ void l_function_params(int lvar __attribute__((lockable))); // \
   expected-warning {{'lockable' attribute only applies to classes}}
 
 
-/***********************************
- *  Scoped Lockable Attribute (sl)
- ***********************************/
+//-----------------------------------------//
+//  Scoped Lockable Attribute (sl)
+//-----------------------------------------//
 
 #if !__has_attribute(scoped_lockable)
 #error "Should support scoped_lockable attribute"
@@ -215,11 +244,11 @@ void sl_function_params(int lvar __attribute__((scoped_lockable))); // \
   expected-warning {{'scoped_lockable' attribute only applies to classes}}
 
 
-/***********************************
- *  Guarded By Attribute (gb)
- ***********************************/
+//-----------------------------------------//
+//  Guarded By Attribute (gb)
+//-----------------------------------------//
 
-// FIXME: Would we like this attribute to take more than 1 arg?
+// FIXME: Eventually, would we like this attribute to take more than 1 arg?
 
 #if !__has_attribute(guarded_by)
 #error "Should support guarded_by attribute"
@@ -258,24 +287,36 @@ int gb_testfn(int y){
   return x;
 }
 
-//2.Deal with argument parsing:
-// grab token stream parsing from C++0x branch
-// possibly create new, more permissive category for gcc attributes
-
-//foo
-//foo.bar
-//foo.bar->baz
-//foo.bar()->baz()->a
-//&foo
-//*foo
+//2. Check argument parsing.
+
+// legal attribute arguments
+int gb_var_arg_1 __attribute__((guarded_by(muWrapper.mu)));
+int gb_var_arg_2 __attribute__((guarded_by(muDoubleWrapper.muWrapper->mu)));
+int gb_var_arg_3 __attribute__((guarded_by(muWrapper.getMu())));
+int gb_var_arg_4 __attribute__((guarded_by(*muWrapper.getMuPointer())));
+int gb_var_arg_5 __attribute__((guarded_by(&mu1)));
+int gb_var_arg_6 __attribute__((guarded_by(muRef)));
+int gb_var_arg_7 __attribute__((guarded_by(muDoubleWrapper.getWrapper()->getMu())));
+int gb_var_arg_8 __attribute__((guarded_by(muPointer)));
+
+
+// illegal attribute arguments
+int gb_var_arg_bad_1 __attribute__((guarded_by(1))); // \
+  expected-error {{'guarded_by' attribute requires arguments that are class type or point to class type}}
+int gb_var_arg_bad_2 __attribute__((guarded_by("mu"))); // \
+  expected-error {{'guarded_by' attribute requires arguments that are class type or point to class type}}
+int gb_var_arg_bad_3 __attribute__((guarded_by(muDoublePointer))); // \
+  expected-error {{'guarded_by' attribute requires arguments that are class type or point to class type}}
+int gb_var_arg_bad_4 __attribute__((guarded_by(umu))); // \
+  expected-error {{'guarded_by' attribute requires arguments whose type is annotated with 'lockable' attribute}}
 
 //3.
 // Thread Safety analysis tests
 
 
-/***********************************
- *  Pt Guarded By Attribute (pgb)
- ***********************************/
+//-----------------------------------------//
+//  Pt Guarded By Attribute (pgb)
+//-----------------------------------------//
 
 #if !__has_attribute(pt_guarded_by)
 #error "Should support pt_guarded_by attribute"
@@ -317,12 +358,35 @@ void pgb_testfn(int y){
   delete x;
 }
 
-/***********************************
- *  Acquired After (aa)
- ***********************************/
+//2. Check argument parsing.
+
+// legal attribute arguments
+int * pgb_var_arg_1 __attribute__((pt_guarded_by(muWrapper.mu)));
+int * pgb_var_arg_2 __attribute__((pt_guarded_by(muDoubleWrapper.muWrapper->mu)));
+int * pgb_var_arg_3 __attribute__((pt_guarded_by(muWrapper.getMu())));
+int * pgb_var