[cfe-commits] [Patch] Thread-safety analysis: more refactoring
Delesley Hutchins
delesley at google.com
Fri Oct 21 10:37:46 PDT 2011
Updated patch enclosed, and found at:
http://codereview.appspot.com/5293049/
-DeLesley
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-refactoring-removelocks-invalid-lock2.patch
Type: text/x-patch
Size: 12271 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111021/d633efa3/attachment.bin>
More information about the cfe-commits
mailing list