[cfe-commits] r146174 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/AnalysisBasedWarnings.cpp test/SemaCXX/warn-thread-safety-analysis.cpp

Delesley Hutchins delesley at google.com
Thu Dec 8 12:45:43 PST 2011


I don't like the second warning, but it's not necessarily all that
easy to get rid of it.  As with many warnings, once the compiler is
confused, you may get several warnings due to one mistake.  It's hard
to avoid that in all cases.

  -DeLesley

On Thu, Dec 8, 2011 at 12:32 PM, David Blaikie <dblaikie at gmail.com> wrote:
> On Thu, Dec 8, 2011 at 12:23 PM, DeLesley Hutchins <delesley at google.com> wrote:
>> Author: delesley
>> Date: Thu Dec  8 14:23:06 2011
>> New Revision: 146174
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=146174&view=rev
>> Log:
>> This patch extends thread safety analysis with support for the scoped_lockable attribute.
>>
>> Modified:
>>    cfe/trunk/lib/Analysis/ThreadSafety.cpp
>>    cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
>>    cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
>>
>> Modified: cfe/trunk/lib/Analysis/ThreadSafety.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/ThreadSafety.cpp?rev=146174&r1=146173&r2=146174&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
>> +++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Thu Dec  8 14:23:06 2011
>> @@ -172,6 +172,10 @@
>>   }
>>
>>  public:
>> +  explicit MutexID(clang::Decl::EmptyShell e) {
>> +    DeclSeq.clear();
>> +  }
>> +
>>   /// \param MutexExp The original mutex expression within an attribute
>>   /// \param DeclExp An expression involving the Decl on which the attribute
>>   ///        occurs.
>> @@ -249,9 +253,14 @@
>>   ///
>>   /// FIXME: add support for re-entrant locking and lock up/downgrading
>>   LockKind LKind;
>> +  MutexID UnderlyingMutex;  // for ScopedLockable objects
>>
>>   LockData(SourceLocation AcquireLoc, LockKind LKind)
>> -    : AcquireLoc(AcquireLoc), LKind(LKind) {}
>> +    : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Decl::EmptyShell())
>> +  {}
>> +
>> +  LockData(SourceLocation AcquireLoc, LockKind LKind, const MutexID &Mu)
>> +    : AcquireLoc(AcquireLoc), LKind(LKind), UnderlyingMutex(Mu) {}
>>
>>   bool operator==(const LockData &other) const {
>>     return AcquireLoc == other.AcquireLoc && LKind == other.LKind;
>> @@ -285,11 +294,12 @@
>>   Lockset::Factory &LocksetFactory;
>>
>>   // Helper functions
>> -  void removeLock(SourceLocation UnlockLoc, MutexID &Mutex);
>> -  void addLock(SourceLocation LockLoc, MutexID &Mutex, LockKind LK);
>> +  void addLock(const MutexID &Mutex, const LockData &LDat);
>> +  void removeLock(const MutexID &Mutex, SourceLocation UnlockLoc);
>>
>>   template <class AttrType>
>> -  void addLocksToSet(LockKind LK, AttrType *Attr, Expr *Exp, NamedDecl *D);
>> +  void addLocksToSet(LockKind LK, AttrType *Attr,
>> +                     Expr *Exp, NamedDecl *D, VarDecl *VD = 0);
>>   void removeLocksFromSet(UnlockFunctionAttr *Attr,
>>                           Expr *Exp, NamedDecl* FunDecl);
>>
>> @@ -298,7 +308,7 @@
>>                            Expr *MutexExp, ProtectedOperationKind POK);
>>   void checkAccess(Expr *Exp, AccessKind AK);
>>   void checkDereference(Expr *Exp, AccessKind AK);
>> -  void handleCall(Expr *Exp, NamedDecl *D);
>> +  void handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD = 0);
>>
>>   /// \brief Returns true if the lockset contains a lock, regardless of whether
>>   /// the lock is held exclusively or shared.
>> @@ -342,51 +352,62 @@
>>   void VisitCastExpr(CastExpr *CE);
>>   void VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp);
>>   void VisitCXXConstructExpr(CXXConstructExpr *Exp);
>> +  void VisitDeclStmt(DeclStmt *S);
>>  };
>>
>>  /// \brief Add a new lock to the lockset, warning if the lock is already there.
>> -/// \param LockLoc The source location of the acquire
>> -/// \param LockExp The lock expression corresponding to the lock to be added
>> -void BuildLockset::addLock(SourceLocation LockLoc, MutexID &Mutex,
>> -                           LockKind LK) {
>> -  // FIXME: deal with acquired before/after annotations. We can write a first
>> -  // pass that does the transitive lookup lazily, and refine afterwards.
>> -  LockData NewLock(LockLoc, LK);
>> -
>> +/// \param Mutex -- the Mutex expression for the lock
>> +/// \param LDat  -- the LockData for the lock
>> +void BuildLockset::addLock(const MutexID &Mutex, const LockData& LDat) {
>> +  // FIXME: deal with acquired before/after annotations.
>>   // FIXME: Don't always warn when we have support for reentrant locks.
>>   if (locksetContains(Mutex))
>> -    Handler.handleDoubleLock(Mutex.getName(), LockLoc);
>> +    Handler.handleDoubleLock(Mutex.getName(), LDat.AcquireLoc);
>>   else
>> -    LSet = LocksetFactory.add(LSet, Mutex, NewLock);
>> +    LSet = LocksetFactory.add(LSet, Mutex, LDat);
>>  }
>>
>>  /// \brief Remove a lock from the lockset, warning if the lock is not there.
>>  /// \param LockExp The lock expression corresponding to the lock to be removed
>>  /// \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)
>> +void BuildLockset::removeLock(const MutexID &Mutex, SourceLocation UnlockLoc) {
>> +  const LockData *LDat = LSet.lookup(Mutex);
>> +  if (!LDat)
>>     Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);
>> -  else
>> -    LSet = NewLSet;
>> +  else {
>> +    // For scoped-lockable vars, remove the mutex associated with this var.
>> +    if (LDat->UnderlyingMutex.isValid())
>> +      removeLock(LDat->UnderlyingMutex, UnlockLoc);
>> +    LSet = LocksetFactory.remove(LSet, Mutex);
>> +  }
>>  }
>>
>>  /// \brief This function, parameterized by an attribute type, is used to add a
>>  /// set of locks specified as attribute arguments to the lockset.
>>  template <typename AttrType>
>>  void BuildLockset::addLocksToSet(LockKind LK, AttrType *Attr,
>> -                                 Expr *Exp, NamedDecl* FunDecl) {
>> +                                 Expr *Exp, NamedDecl* FunDecl, VarDecl *VD) {
>>   typedef typename AttrType::args_iterator iterator_type;
>>
>>   SourceLocation ExpLocation = Exp->getExprLoc();
>>
>> +  // Figure out if we're calling the constructor of scoped lockable class
>> +  bool isScopedVar = false;
>> +  if (VD) {
>> +    if (CXXConstructorDecl *CD = dyn_cast<CXXConstructorDecl>(FunDecl)) {
>> +      CXXRecordDecl* PD = CD->getParent();
>> +      if (PD && PD->getAttr<ScopedLockableAttr>())
>> +        isScopedVar = true;
>> +    }
>> +  }
>> +
>>   if (Attr->args_size() == 0) {
>>     // The mutex held is the "this" object.
>>     MutexID Mutex(0, Exp, FunDecl);
>>     if (!Mutex.isValid())
>>       MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
>>     else
>> -      addLock(ExpLocation, Mutex, LK);
>> +      addLock(Mutex, LockData(ExpLocation, LK));
>>     return;
>>   }
>>
>> @@ -394,8 +415,15 @@
>>     MutexID Mutex(*I, Exp, FunDecl);
>>     if (!Mutex.isValid())
>>       MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>> -    else
>> -      addLock(ExpLocation, Mutex, LK);
>> +    else {
>> +      addLock(Mutex, LockData(ExpLocation, LK));
>> +      if (isScopedVar) {
>> +        // For scoped lockable vars, map this var to its underlying mutex.
>> +        DeclRefExpr DRE(VD, VD->getType(), VK_LValue, VD->getLocation());
>> +        MutexID SMutex(&DRE, 0, 0);
>> +        addLock(SMutex, LockData(VD->getLocation(), LK, Mutex));
>> +      }
>> +    }
>>   }
>>  }
>>
>> @@ -412,7 +440,7 @@
>>     if (!Mu.isValid())
>>       MutexID::warnInvalidLock(Handler, 0, Exp, FunDecl);
>>     else
>> -      removeLock(ExpLocation, Mu);
>> +      removeLock(Mu, ExpLocation);
>>     return;
>>   }
>>
>> @@ -422,7 +450,7 @@
>>     if (!Mutex.isValid())
>>       MutexID::warnInvalidLock(Handler, *I, Exp, FunDecl);
>>     else
>> -      removeLock(ExpLocation, Mutex);
>> +      removeLock(Mutex, ExpLocation);
>>   }
>>  }
>>
>> @@ -508,7 +536,7 @@
>>  ///
>>  /// FIXME: Do not flag an error for member variables accessed in constructors/
>>  /// destructors
>> -void BuildLockset::handleCall(Expr *Exp, NamedDecl *D) {
>> +void BuildLockset::handleCall(Expr *Exp, NamedDecl *D, VarDecl *VD) {
>>   AttrVec &ArgAttrs = D->getAttrs();
>>   for(unsigned i = 0; i < ArgAttrs.size(); ++i) {
>>     Attr *Attr = ArgAttrs[i];
>> @@ -517,7 +545,7 @@
>>       // to our lockset with kind exclusive.
>>       case attr::ExclusiveLockFunction: {
>>         ExclusiveLockFunctionAttr *A = cast<ExclusiveLockFunctionAttr>(Attr);
>> -        addLocksToSet(LK_Exclusive, A, Exp, D);
>> +        addLocksToSet(LK_Exclusive, A, Exp, D, VD);
>>         break;
>>       }
>>
>> @@ -525,7 +553,7 @@
>>       // to our lockset with kind shared.
>>       case attr::SharedLockFunction: {
>>         SharedLockFunctionAttr *A = cast<SharedLockFunctionAttr>(Attr);
>> -        addLocksToSet(LK_Shared, A, Exp, D);
>> +        addLocksToSet(LK_Shared, A, Exp, D, VD);
>>         break;
>>       }
>>
>> @@ -627,10 +655,23 @@
>>  }
>>
>>  void BuildLockset::VisitCXXConstructExpr(CXXConstructExpr *Exp) {
>> -  NamedDecl *D = cast<NamedDecl>(Exp->getConstructor());
>> -  if(!D || !D->hasAttrs())
>> -    return;
>> -  handleCall(Exp, D);
>> +  // FIXME -- only handles constructors in DeclStmt below.
>> +}
>> +
>> +void BuildLockset::VisitDeclStmt(DeclStmt *S) {
>> +  DeclGroupRef DGrp = S->getDeclGroup();
>> +  for (DeclGroupRef::iterator I = DGrp.begin(), E = DGrp.end(); I != E; ++I) {
>> +    Decl *D = *I;
>> +    if (VarDecl *VD = dyn_cast_or_null<VarDecl>(D)) {
>> +      Expr *E = VD->getInit();
>> +      if (CXXConstructExpr *CE = dyn_cast_or_null<CXXConstructExpr>(E)) {
>> +        NamedDecl *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
>> +        if (!CtorD || !CtorD->hasAttrs())
>> +          return;
>> +        handleCall(CE, CtorD, VD);
>> +      }
>> +    }
>> +  }
>>  }
>>
>>
>>
>> Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=146174&r1=146173&r2=146174&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)
>> +++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Thu Dec  8 14:23:06 2011
>> @@ -834,8 +834,8 @@
>>   // prototyping, but we need a way for analyses to say what expressions they
>>   // expect to always be CFGElements and then fill in the BuildOptions
>>   // appropriately.  This is essentially a layering violation.
>> -  if (P.enableCheckUnreachable) {
>> -    // Unreachable code analysis requires a linearized CFG.
>> +  if (P.enableCheckUnreachable || P.enableThreadSafetyAnalysis) {
>> +    // Unreachable code analysis and thread safety require a linearized CFG.
>>     AC.getCFGBuildOptions().setAllAlwaysAdd();
>>   }
>>   else {
>>
>> Modified: cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp?rev=146174&r1=146173&r2=146174&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Thu Dec  8 14:23:06 2011
>> @@ -36,6 +36,18 @@
>>   void LockWhen(const int &cond) __attribute__((exclusive_lock_function));
>>  };
>>
>> +class __attribute__((scoped_lockable)) MutexLock {
>> + public:
>> +  MutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu)));
>> +  ~MutexLock() __attribute__((unlock_function));
>> +};
>> +
>> +class __attribute__((scoped_lockable)) ReaderMutexLock {
>> + public:
>> +  ReaderMutexLock(Mutex *mu) __attribute__((exclusive_lock_function(mu)));
>> +  ~ReaderMutexLock() __attribute__((unlock_function));
>> +};
>> +
>>
>>  Mutex sls_mu;
>>
>> @@ -1549,3 +1561,47 @@
>>   template struct W<int>; // expected-note {{here}}
>>
>>  }
>> +
>> +namespace test_scoped_lockable {
>> +
>> +struct TestScopedLockable {
>> +  Mutex mu1;
>> +  Mutex mu2;
>> +  int a __attribute__((guarded_by(mu1)));
>> +  int b __attribute__((guarded_by(mu2)));
>> +
>> +  bool getBool();
>> +
>> +  void foo1() {
>> +    MutexLock mulock(&mu1);
>> +    a = 5;
>> +  }
>> +
>> +  void foo2() {
>> +    ReaderMutexLock mulock1(&mu1);
>> +    if (getBool()) {
>> +      MutexLock mulock2a(&mu2);
>> +      b = a + 1;
>> +    }
>> +    else {
>> +      MutexLock mulock2b(&mu2);
>> +      b = a + 2;
>> +    }
>> +  }
>> +
>> +  void foo3() {
>> +    MutexLock mulock_a(&mu1);
>> +    MutexLock mulock_b(&mu1); // \
>> +      // expected-warning {{locking 'mu1' that is already locked}}
>> +  }   // expected-warning {{unlocking 'mu1' that was not locked}}
>
> I assume at some point this could be extended/improved to not provide
> that second warning so the user is directed to the relevant issue at
> the MutexLock construction?
>
> - David
>
>> +
>> +  void foo4() {
>> +    MutexLock mulock1(&mu1), mulock2(&mu2);
>> +    a = b+1;
>> +    b = a+1;
>> +  }
>> +};
>> +
>> +} // end namespace test_scoped_lockable
>> +
>> +
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits



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




More information about the cfe-commits mailing list