[cfe-commits] [cfe-dev] Proposal for thread safety attributes for Clang

[Douglas Gregor]

On Aug 1, 2011, at 4:17 PM, Caitlin Sadowski wrote:

> Here it is.
> 
> Cheers,


I don't see the reason for this change:

--- a/lib/Sema/SemaDeclAttr.cpp
+++ b/lib/Sema/SemaDeclAttr.cpp
@@ -27,7 +27,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 {
   ExpectedFunction,
   ExpectedUnion,
   ExpectedVariableOrFunction,

A few code style nits here:

+
+/// \brief Thread Safety Analysis: Check if passed in RecordType was annotated
+/// with the 'lockable' attribute. Argument should be non-null.
+static bool isLockable(const RecordType * RT) {
+  RecordDecl * RD = RT->getDecl();

LLVM style generally puts the '*' next to the declarator-id, e.g.,

	RecordDecl *RD

+
+  if(!RD->hasAttrs())
+    return false;
+
+  AttrVec & ArgAttrs = RD->getAttrs();
+  for(unsigned int i = 0;i < ArgAttrs.size();++i) {
+    if(isa < LockableAttr > (ArgAttrs[i]))

Usually written as isa<LockableAttr>(ArgAttrs[i])

+/// \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);
+    QualType Arg_QT = ArgExp->getType();

You probably want to check whether ArgExpr->isTypeDependent() here, and then just "continue" if it is, because when the argument type is dependent, you'll have no way to determine whether it's a lockable type or not.

+    // Get record type.
+    // first see if we can just cast to record type
+    const RecordType * RT = Arg_QT->getAs<RecordType>();
+
+    // now check if we point to record type
+    if (!RT && Arg_QT->isPointerType()) {
+        QualType PT = Arg_QT->getAs<PointerType>()->getPointeeType();
+        RT = PT->getAs<RecordType>();
+    }
+
+    // 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);

You want exactly integer literals here? No extra parentheses, no "1+2", just an integer literal?

+      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;
+        }
+        RT = FD->getParamDecl(ParamIdx_from0)->getType()->getAs<RecordType>();

Don't you want to look through parameters of reference or pointer type here?

+      }
+    }
+
+    //  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 (!isLockable(RT)) {
+      S.Diag(Attr.getLoc(), diag::err_attribute_argument_not_lockable)
+        << Attr.getName();
+      return false;
+    }
+  }
+  return true;
+}

+/// \brief Thread Safety Analysis: Check if passed in RecordType was annotated
+/// with the 'lockable' attribute. Argument should be non-null.
+static bool isLockable(const RecordType * RT) {
+  RecordDecl * RD = RT->getDecl();
+
+  if(!RD->hasAttrs())
+    return false;
+
+  AttrVec & ArgAttrs = RD->getAttrs();
+  for(unsigned int i = 0;i < ArgAttrs.size();++i) {
+    if(isa < LockableAttr > (ArgAttrs[i]))
+      return true;
+  }

This whole function collapses down to 

	RT->getDecl()->getAttr<LockableAttr>();

@@ -371,6 +441,21 @@ static void handleAcquireOrderAttr(Sema &S, Decl *D, const AttributeList &Attr,
     return;
   }
 
+  // check that this attribute only applies to lockable types
+  if (ValueDecl *VD = dyn_cast<ValueDecl>(D)) {
+    if (const RecordType * RT = VD->getType()->getAs<RecordType>()) {
+      if (!isLockable(RT)) {
+        S.Diag(Attr.getLoc(), diag::err_attribute_decl_not_lockable)
+          << Attr.getName();
+        return;
+      }
+    }
+  }

The comment and code don't agree. The code is checking that the type of the value is either not a RecordType or is a lockable record type.

If you do intend this to do what the comment says, you should also allow dependent types (which occur in templates; the attribute will need to get checked again when the variable is instantiated). Also, do you want to permit variables of reference-to-lockable or pointer-to-lockable type?

-  if (!isFunction(D)) {
+  if (!isa<FunctionDecl>(D)) {

This happens a number of times. You probably also want to allow FunctionTemplateDecl, and at some point we'll also want ObjCMethodDecl.

	- Doug