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

[Delesley Hutchins]

Test case at:

http://codereview.appspot.com/5310043/diff2/5001:7001/test/SemaCXX/warn-thread-safety-analysis.cpp

  -DeLesley

On Fri, Oct 21, 2011 at 10:34 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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
>>
>>
>
>



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