[cfe-commits] [PATCH] Thread-safety analysis: handle attributes on constructors

[Richard Smith]

Hi,

On Wed, October 19, 2011 00:15, Delesley Hutchins wrote:
> This patch adds support for handling thread safety attributes on
> constructors as well as ordinary methods.
>
> http://codereview.appspot.com/5310043/
>
> Note to reviewers: the patch file is a bit misleading.  The main
> change is that I moved implementation of VisitCXXMemberCallExpr into a separate
> function (handleCall), that is invoked from both VisitCXXMemberCallExpr and
> VisitCXXConstructExpr.  However, diff
> thinks I moved around other things instead.

--- a/lib/Analysis/ThreadSafety.cpp
+++ b/lib/Analysis/ThreadSafety.cpp
@@ -182,10 +182,13 @@ class MutexID {
         buildMutexID(Parent, 0, 0, 0, 0);
       else
         return;  // mutexID is still valid in this case
-    } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp))
+    } else if (UnaryOperator *UOE = dyn_cast<UnaryOperator>(Exp)) {
+      buildMutexID(UOE->getSubExpr(), Parent, NumArgs, FunArgDecls, FunArgs);

Do you want to be this liberal? UO_AddrOf and UO_Deref seem like good things
to handle (and UO_Plus and UO_Extension seem harmless), but
increment/decrement in a mutex expression seems like something that should be
blacklisted.

+    } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp)) {
       buildMutexID(CE->getSubExpr(), Parent, NumArgs, FunArgDecls, FunArgs);
-    else
+    } else {
       DeclSeq.clear(); // Mark as invalid lock expression.
+    }

The prevalent style in clang is to omit the braces for these ifs.

   }

   /// \brief Construct a MutexID from an expression.
@@ -210,6 +213,10 @@ class MutexID {
       Parent = CE->getImplicitObjectArgument();
       NumArgs = CE->getNumArgs();
       FunArgs = CE->getArgs();
+    } else if (CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(DeclExp)) {
+      Parent = 0;  // FIXME -- is there a way to get the parent?

CXXConstructExpr doesn't know which Decl (if any) it's initializing. If you
want to be able to handle things like:

struct C { Mutex Mu; C() EXCLUSIVE_LOCK_FUNCTION(this->Mu); /*...*/ };
C c; // c.Mu is known to be acquired

... then I think you'll need to visit DeclStmts, trawl their declaration group
looking for initialized VarDecls, and build up some sort of mapping yourself.

+      NumArgs = CE->getNumArgs();
+      FunArgs = CE->getArgs();
     }

     // If the attribute has no arguments, then assume the argument is "this".

The rest of the patch looks fine.

Thanks!
Richard