[cfe-commits] [Patch] Thread-safety analysis: more refactoring

Richard Smith richard at metafoo.co.uk
Fri Oct 21 10:56:33 PDT 2011


On Fri, October 21, 2011 18:37, Delesley Hutchins wrote:
> Updated patch enclosed, and found at:
>
> http://codereview.appspot.com/5293049/

Please capitalize the comment for BuildLockset::removeLocksFromSet, then feel
free to check in.

Thanks!

> On Thu, Oct 20, 2011 at 7:51 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Wed, October 19, 2011 23:58, Delesley Hutchins wrote:
>>> This patch performs two additional refactorings.  The first refactors
>>> lockset removal into a separate function, to be consistent with lockset
>>> addition.  The second refactors the handling of invalid lock expressions
>>> into a separate routine, in preparation for better handling of such
>>> messages in more complex cases, such as attributes on destructors.  There
>>> is no major change in functionality.
>>>
>>> http://codereview.appspot.com/5293049/
>>>
>>
>> Some comments below.
>>
>>
>> --- a/lib/Analysis/ThreadSafety.cpp
>> +++ b/lib/Analysis/ThreadSafety.cpp
>> @@ -161,6 +161,11 @@ class MutexID {
>>   /// Recursive function that bottoms out when the final DeclRefExpr is
>> reached.   void buildMutexID(Expr *Exp, Expr *Parent, int NumArgs,
>>                     const NamedDecl **FunArgDecls, Expr **FunArgs) {
>> +    if (!Exp) {
>> +      DeclSeq.clear();
>> +      return;
>> +    }
>>
>>
>> Can this happen? If this is a crash fix, there should be a testcase,
>> otherwise this should be an assert.
>>
>> +
>>     if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Exp)) {
>>       if (FunArgDecls) {
>>         // Substitute call arguments for references to function parameters
>> @@ -202,11 +207,13 @@ class MutexID {
>>     Expr **FunArgs = 0;
>>     SmallVector<const NamedDecl*, 8> FunArgDecls;
>>
>>
>> +    // If we are processing a raw attribute expression...
>>     if (DeclExp == 0) {
>>       buildMutexID(MutexExp, 0, 0, 0, 0);
>>       return;
>>     }
>>
>>
>> +    // Get Parent and FunArgs from DeclExp
>>
>>
>> It'd be nice if this comment explained why we're doing this. As an outsider
>> to this code, it wasn't immediately obvious to me that we want to extract
>> these for substitution.
>>
>>     if (MemberExpr *ME = dyn_cast<MemberExpr>(DeclExp)) {
>>       Parent = ME->getBase();
>>     } else if (CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(DeclExp))
>> {
>> @@ -250,6 +257,18 @@ public:
>>     return !DeclSeq.empty();
>>   }
>>
>>
>> +  /// Issue a warning about an invalid lock expression
>> +  static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,
>>  +                              Expr *DeclExp, const NamedDecl* D) {
>> +    SourceLocation Loc;
>> +    if (DeclExp)
>> +      Loc = DeclExp->getExprLoc();
>> +
>> +    // FIXME: add a note about the attribute location in MutexExp or D
>> +    if (Loc.isValid())
>> +      Handler.handleInvalidLockExp(Loc);
>> +  }
>> +
>>   bool operator==(const MutexID &other) const {
>>     return DeclSeq == other.DeclSeq;
>>   }
>> @@ -329,6 +348,8 @@ typedef llvm::ImmutableMap<MutexID, LockData> Lockset;
>>  /// output error messages related to missing locks.
>>  /// FIXME: In future, we may be able to not inherit from a visitor.
>>  class BuildLockset : public StmtVisitor<BuildLockset> {
>> +  friend class ThreadSafetyAnalyzer;
>> +
>>   ThreadSafetyHandler &Handler;
>>   Lockset LSet;
>>   Lockset::Factory &LocksetFactory;
>> @@ -339,6 +360,8 @@ class BuildLockset : public StmtVisitor<BuildLockset> {
>>
>>
>>   template <class AttrType>
>>   void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D);
>> +  void removeLocksFromSet(UnlockFunctionAttr *Attr,
>> +                          Expr *Exp, NamedDecl* FunDecl);
>>
>>
>>   const ValueDecl *getValueDecl(Expr *Exp);
>>   void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,
>> @@ -401,9 +424,10 @@ void BuildLockset::addLock(SourceLocation LockLoc,
>> MutexID &Mutex,
>>   LockData NewLock(LockLoc, LK);
>>
>>
>>   // FIXME: Don't always warn when we have support for reentrant locks.
>> -  if (locksetContains(Mutex))
>> +  if (locksetContains(Mutex) && LockLoc.isValid())
>>
>>
>> What's the meaning of an invalid LockLoc? A test or an assert would
>> elucidate.
>>
>>     Handler.handleDoubleLock(Mutex.getName(), LockLoc);
>> -  LSet = LocksetFactory.add(LSet, Mutex, NewLock);
>> +  else
>> +    LSet = LocksetFactory.add(LSet, Mutex, NewLock);
>>  }
>>
>>
>>  /// \brief Remove a lock from the lockset, warning if the lock is not
>> there. @@ -411,10 +435,10 @@ void BuildLockset::addLock(SourceLocation
>> LockLoc,
>> MutexID &Mutex,
>>  /// \param UnlockLoc The source location of the unlock (only used in error
>> msg)  void BuildLockset::removeLock(SourceLocation UnlockLoc, MutexID
>> &Mutex) {
>>   Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);
>> -  if(NewLSet == LSet)
>> +  if(NewLSet == LSet && UnlockLoc.isValid())
>>     Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
>> -
>> -  LSet = NewLSet;
>> +  else
>> +    LSet = NewLSet;
>>  }
>>
>>
>>  /// \brief This function, parameterized by an attribute type, is used to
>> add a @@ -428,10 +452,9 @@ void BuildLockset::addLocksToSet(LockKind LK,
>> AttrType
>> *Attr,
>>
>>
>>   if (Attr->args_size() == 0) {
>>     // The mutex held is the "this" object.
>> -
>>     MutexID Mutex(0, Exp, FunDecl);
>>     if (!Mutex.isValid())
>> -      Handler.handleInvalidLockExp(Exp->getExprLoc());
>> +      MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
>>     else
>>       addLock(ExpLocation, Mutex, LK);
>>     return;
>> @@ -440,12 +463,39 @@ void BuildLockset::addLocksToSet(LockKind LK, AttrType
>>  *Attr,
>>   for (iterator_type I=Attr->args_begin(), E=Attr->args_end(); I != E; ++I)
>> {
>>     MutexID Mutex(*I, Exp, FunDecl);
>>     if (!Mutex.isValid())
>> -      Handler.handleInvalidLockExp(Exp->getExprLoc());
>> +      MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>>     else
>>       addLock(ExpLocation, Mutex, LK);
>>   }
>>  }
>>
>>
>> +/// \brief this function adds a set of locks specified as attribute
>> arguments +/// to the lockset.
>>
>>
>> This comment needs updating.
>>
>>
>> +void BuildLockset::removeLocksFromSet(UnlockFunctionAttr *Attr,
>> +                                      Expr *Exp, NamedDecl* FunDecl) {
>> +  SourceLocation ExpLocation;
>> +  if (Exp) ExpLocation = Exp->getExprLoc();
>> +
>> +  if (Attr->args_size() == 0) {
>> +    // The mutex held is the "this" object.
>> +    MutexID Mu(0, Exp, FunDecl);
>> +    if (!Mu.isValid())
>> +      MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
>> +    else
>> +      removeLock(ExpLocation, Mu);
>> +    return;
>> +  }
>> +
>> +  for (UnlockFunctionAttr::args_iterator I = Attr->args_begin(),
>> +       E = Attr->args_end(); I != E; ++I) {
>> +    MutexID Mutex(*I, Exp, FunDecl);
>> +    if (!Mutex.isValid())
>> +      MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>> +    else
>> +      removeLock(ExpLocation, Mutex);
>> +  }
>> +}
>> +
>>  /// \brief Gets the value decl pointer from DeclRefExprs or MemberExprs
>>  const ValueDecl *BuildLockset::getValueDecl(Expr *Exp) {
>>   if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(Exp))
>> @@ -466,7 +516,7 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl
>> *D,
>> Expr *Exp,
>>
>>
>>   MutexID Mutex(MutexExp, Exp, D);
>>   if (!Mutex.isValid())
>> -    Handler.handleInvalidLockExp(MutexExp->getExprLoc());
>> +    MutexID::warnInvalidLock(Handler, MutexExp, Exp, D);
>>   else if (!locksetContainsAtLeast(Mutex, LK))
>>     Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK,
>> Exp->getExprLoc());
>>  }
>> @@ -529,8 +579,6 @@ void BuildLockset::checkAccess(Expr *Exp, AccessKind AK)
>> {
>>  /// FIXME: Do not flag an error for member variables accessed in
>> constructors/  /// destructors
>>  void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
>> -  SourceLocation ExpLocation = Exp->getExprLoc();
>> -
>>   AttrVec &ArgAttrs = D->getAttrs();
>>   for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
>>     Attr *Attr = ArgAttrs[i];
>> @@ -555,24 +603,7 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl *D)
>> {
>>       // mutexes from the lockset, and flag a warning if they are not there.
>>        case attr::UnlockFunction: {
>>         UnlockFunctionAttr *UFAttr = cast<UnlockFunctionAttr>(Attr);
>> -
>> -        if (UFAttr->args_size() == 0) { // The lock held is the "this"
>> object. -          MutexID Mu(0, Exp, D);
>> -          if (!Mu.isValid())
>> -            Handler.handleInvalidLockExp(Exp->getExprLoc());
>> -          else
>> -            removeLock(ExpLocation, Mu);
>> -          break;
>> -        }
>> -
>> -        for (UnlockFunctionAttr::args_iterator I = UFAttr->args_begin(),
>> -             E = UFAttr->args_end(); I != E; ++I) {
>> -          MutexID Mutex(*I, Exp, D);
>> -          if (!Mutex.isValid())
>> -            Handler.handleInvalidLockExp(Exp->getExprLoc());
>> -          else
>> -            removeLock(ExpLocation, Mutex);
>> -        }
>> +        removeLocksFromSet(UFAttr, Exp, D);
>>         break;
>>       }
>>
>>
>> @@ -601,10 +632,10 @@ void BuildLockset::handleCall(Expr *Exp, NamedDecl
>> *D) {
>>             E = LEAttr->args_end(); I != E; ++I) {
>>           MutexID Mutex(*I, Exp, D);
>>           if (!Mutex.isValid())
>> -            Handler.handleInvalidLockExp((*I)->getExprLoc());
>> +            MutexID::warnInvalidLock(Handler, *I, Exp, D);
>>           else if (locksetContains(Mutex))
>>             Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),
>> -                                          ExpLocation);
>> +                                          Exp->getExprLoc());
>>         }
>>         break;
>>       }
>> @@ -737,7 +768,7 @@ Lockset ThreadSafetyAnalyzer::addLock(Lockset &LSet,
>> Expr
>> *MutexExp,
>>                                       LockKind LK, SourceLocation Loc) {
>>   MutexID Mutex(MutexExp, 0, D);
>>   if (!Mutex.isValid()) {
>> -    Handler.handleInvalidLockExp(MutexExp->getExprLoc());
>> +    MutexID::warnInvalidLock(Handler, MutexExp, 0, D);
>>     return LSet;
>>   }
>>   LockData NewLock(Loc, LK);
>>
>>
>>
>>
>
>
>
> --
> DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
>
>




More information about the cfe-commits mailing list