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

[Richard Smith]

On Fri, October 21, 2011 17:42, Delesley Hutchins wrote:
> Updated patch now at:
>
>
> http://codereview.appspot.com/5310043/

Thanks! Please add some tests. The patch itself looks fine.

Richard

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