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

[Delesley Hutchins]

Updated patch now at:

  http://codereview.appspot.com/5310043/

  -DeLesley

On Thu, Oct 20, 2011 at 7:07 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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
>
>



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315