Issue 4960064: Adding support for unparseable locks
Patch Set 1 #Patch Set 2 : Cleaned up slightly #
Total comments: 6
Patch Set 3 : Incorporating chandler comments #Patch Set 4 : Fixed version #
Total comments: 6
MessagesTotal messages: 6
http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (left): http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:159: Why the formatting change? (i find it makes the code less readable) http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (right): http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:181: bool Valid; Rather than a public bool member, I would provide an isValid() method. Also, consider providing an operator bool() for use in conditions. http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:189: Handler.handleInvalidLockExp(LExpr->getExprLoc()); The more I see this, the more I question the design. Why can't we leave it as the responsibility of the code that builds a new ID to diagnose invalid IDs using the isValid() method? Fundamentally, the constructor for a MutexID accepting a ThreadSafetyHandler doesn't make much sense to me.
Sign in to reply to this message.
Let me know if the new draft is suitable to commit. http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (left): http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:159: On 2011/09/12 04:34:21, chandlerc wrote: > Why the formatting change? (i find it makes the code less readable) Done. http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (right): http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:181: bool Valid; isValid method provided. On 2011/09/12 04:34:21, chandlerc wrote: > Rather than a public bool member, I would provide an isValid() method. Also, > consider providing an operator bool() for use in conditions. http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:189: Handler.handleInvalidLockExp(LExpr->getExprLoc()); On 2011/09/12 04:34:21, chandlerc wrote: > The more I see this, the more I question the design. Why can't we leave it as > the responsibility of the code that builds a new ID to diagnose invalid IDs > using the isValid() method? > > Fundamentally, the constructor for a MutexID accepting a ThreadSafetyHandler > doesn't make much sense to me. Good point. The isValid method makes it possible to pull this out, and I also prefer it that way. Changed.
Sign in to reply to this message.
Actually, that one was hilariously broken. Here is a better version. Cheers, Caitlin On Mon, Sep 12, 2011 at 10:15 AM, <supertri@google.com> wrote: > Reviewers: chandlerc, > > Message: > Let me know if the new draft is suitable to commit. > > > http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp > File lib/Analysis/ThreadSafety.cpp (left): > > http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... > lib/Analysis/ThreadSafety.cpp:159: > On 2011/09/12 04:34:21, chandlerc wrote: >> >> Why the formatting change? (i find it makes the code less readable) > > Done. > > http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp > File lib/Analysis/ThreadSafety.cpp (right): > > http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... > lib/Analysis/ThreadSafety.cpp:181: bool Valid; > isValid method provided. > > On 2011/09/12 04:34:21, chandlerc wrote: >> >> Rather than a public bool member, I would provide an isValid() method. > > Also, >> >> consider providing an operator bool() for use in conditions. > > http://codereview.appspot.com/4960064/diff/2001/lib/Analysis/ThreadSafety.cpp... > lib/Analysis/ThreadSafety.cpp:189: > Handler.handleInvalidLockExp(LExpr->getExprLoc()); > On 2011/09/12 04:34:21, chandlerc wrote: >> >> The more I see this, the more I question the design. Why can't we > > leave it as >> >> the responsibility of the code that builds a new ID to diagnose > > invalid IDs >> >> using the isValid() method? > >> Fundamentally, the constructor for a MutexID accepting a > > ThreadSafetyHandler >> >> doesn't make much sense to me. > > Good point. The isValid method makes it possible to pull this out, and I > also prefer it that way. Changed. > > > > Please review this at http://codereview.appspot.com/4960064/ > > Affected files: > M lib/Analysis/ThreadSafety.cpp > M test/SemaCXX/warn-thread-safety-analysis.cpp > > > Index: lib/Analysis/ThreadSafety.cpp > diff --git a/lib/Analysis/ThreadSafety.cpp b/lib/Analysis/ThreadSafety.cpp > index > 297928dd97ccbe1d67ee67e2448ae3f549526f3d..2e9e35e4d94e26b3329b6eeb0f07dc4ff669672a > 100644 > --- a/lib/Analysis/ThreadSafety.cpp > +++ b/lib/Analysis/ThreadSafety.cpp > @@ -154,7 +154,9 @@ public: > /// foo(MyL); // requires lock MyL->Mu to be held > class MutexID { > SmallVector<NamedDecl*, 2> DeclSeq; > - ThreadSafetyHandler &Handler; > + /// If we could not properly build the DeclSeq, we mark this lock as not > Valid > + /// and do not use it later. > + bool Valid; > > /// Build a Decl sequence representing the lock from the given expression. > /// Recursive function that bottoms out when the final DeclRefExpr is > reached. > @@ -166,21 +168,24 @@ class MutexID { > NamedDecl *ND = ME->getMemberDecl(); > DeclSeq.push_back(ND); > buildMutexID(ME->getBase(), Parent); > - } else if (isa<CXXThisExpr>(Exp)) { > - if (!Parent) > - return; > + } else if (isa<CXXThisExpr>(Exp) && Parent) { > buildMutexID(Parent, 0); > } else if (CastExpr *CE = dyn_cast<CastExpr>(Exp)) > buildMutexID(CE->getSubExpr(), Parent); > else > - Handler.handleInvalidLockExp(Exp->getExprLoc()); > + Valid = false; > + Valid = true; > } > > public: > - MutexID(ThreadSafetyHandler &Handler, Expr *LExpr, Expr *ParentExpr) > - : Handler(Handler) { > + MutexID(Expr *LExpr, Expr *ParentExpr) { > buildMutexID(LExpr, ParentExpr); > - assert(!DeclSeq.empty()); > + if (DeclSeq.empty()) > + Valid = false; > + } > + > + bool isValid() { > + return Valid; > } > > bool operator==(const MutexID &other) const { > @@ -206,6 +211,7 @@ public: > /// name in diagnostics is a way to get simple, and consistent, mutex > names. > /// We do not want to output the entire expression text for security > reasons. > StringRef getName() const { > + assert(Valid); > return DeclSeq.front()->getName(); > } > > @@ -324,7 +330,12 @@ public: > void BuildLockset::addLock(SourceLocation LockLoc, Expr *LockExp, Expr > *Parent, > LockKind LK) { > // FIXME: deal with acquired before/after annotations > - MutexID Mutex(Handler, LockExp, Parent); > + MutexID Mutex(LockExp, Parent); > + if (!Mutex.isValid()) { > + Handler.handleInvalidLockExp(LockExp->getExprLoc()); > + return; > + } > + > LockData NewLock(LockLoc, LK); > > // FIXME: Don't always warn when we have support for reentrant locks. > @@ -338,7 +349,11 @@ void BuildLockset::addLock(SourceLocation LockLoc, Expr > *LockExp, Expr *Parent, > /// \param UnlockLoc The source location of the unlock (only used in error > msg) > void BuildLockset::removeLock(SourceLocation UnlockLoc, Expr *LockExp, > Expr *Parent) { > - MutexID Mutex(Handler, LockExp, Parent); > + MutexID Mutex(LockExp, Parent); > + if (!Mutex.isValid()) { > + Handler.handleInvalidLockExp(LockExp->getExprLoc()); > + return; > + } > > Lockset NewLSet = LocksetFactory.remove(LSet, Mutex); > if(NewLSet == LSet) > @@ -365,8 +380,10 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl > *D, Expr *Exp, > ProtectedOperationKind POK) { > LockKind LK = getLockKindFromAccessKind(AK); > Expr *Parent = getParent(Exp); > - MutexID Mutex(Handler, MutexExp, Parent); > - if (!locksetContainsAtLeast(Mutex, LK)) > + MutexID Mutex(MutexExp, Parent); > + if (!Mutex.isValid()) > + Handler.handleInvalidLockExp(MutexExp->getExprLoc()); > + else if (!locksetContainsAtLeast(Mutex, LK)) > Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, > Exp->getExprLoc()); > } > > @@ -556,8 +573,10 @@ void > BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) { > LocksExcludedAttr *LEAttr = cast<LocksExcludedAttr>(Attr); > for (LocksExcludedAttr::args_iterator I = LEAttr->args_begin(), > E = LEAttr->args_end(); I != E; ++I) { > - MutexID Mutex(Handler, *I, Parent); > - if (locksetContains(Mutex)) > + MutexID Mutex(*I, Parent); > + if (!Mutex.isValid()) > + Handler.handleInvalidLockExp((*I)->getExprLoc()); > + else if (locksetContains(Mutex)) > Handler.handleFunExcludesLock(D->getName(), Mutex.getName(), > ExpLocation); > } > Index: test/SemaCXX/warn-thread-safety-analysis.cpp > diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp > b/test/SemaCXX/warn-thread-safety-analysis.cpp > index > 85de91853e599632c284288d7c2c8a32fec5c61a..a3f37ef8b8e3bc99069487c6420ef8ee751a33aa > 100644 > --- a/test/SemaCXX/warn-thread-safety-analysis.cpp > +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp > @@ -701,3 +701,31 @@ void es_bad_7() { > // expected-warning {{cannot call function 'le_fun' while holding mutex > 'sls_mu'}} > sls_mu.Unlock(); > } > + > +//-----------------------------------------------// > +// Unparseable lock expressions > +// ----------------------------------------------// > + > +Mutex UPmu; > +// FIXME: add support for lock expressions involving arrays. > +Mutex mua[5]; > + > +int x __attribute__((guarded_by(UPmu = sls_mu))); // \ > + // expected-warning{{cannot resolve lock expression to a specific > lockable object}} > +int y __attribute__((guarded_by(mua[0]))); // \ > + // expected-warning{{cannot resolve lock expression to a specific > lockable object}} > + > + > +void testUnparse() { > + // no errors, since the lock expressions are not resolved > + x = 5; > + y = 5; > +} > + > +void testUnparse2() { > + mua[0].Lock(); // \ > + // expected-warning{{cannot resolve lock expression to a specific > lockable object}} > + (&(mua[0]) + 4)->Lock(); // \ > + // expected-warning{{cannot resolve lock expression to a specific > lockable object}} > +} > + > > >
Sign in to reply to this message.
Fundamentally, you still need to add tests that produce errors because of invalid locks. There has to be something we can put here to exercise the behavior you're adding. Other than that, and some tweaks below, I think it looks fine... Naturally feel free to mail this upstream whenever, or to address these comments in follow-up commits. http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (right): http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:175: return; Why bother to return or have an else? Alternatively make 171 read 'else if (isa<...>(Exp) && Parent)' http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:185: if (DeclSeq.empty()) I'm still a bit surprised we can't just impliment isValid() as 'DeclSeq.empty()' and omit the bool entirely.
Sign in to reply to this message.
There are test cases in this patch, what should change in them? http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (right): http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:175: return; On 2011/09/13 23:16:20, chandlerc wrote: > Why bother to return or have an else? > > Alternatively make 171 read 'else if (isa<...>(Exp) && Parent)' Note that this would have different behaviour, because it would result in Valid being set to false. In particular, when analyzing inline functions, the this expressions are compared without parents. http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:185: if (DeclSeq.empty()) On 2011/09/13 23:16:20, chandlerc wrote: > I'm still a bit surprised we can't just impliment isValid() as 'DeclSeq.empty()' > and omit the bool entirely. There are two causes of unparseable lock expressions: 1) empty decl sequence (recursion bottomed out without going anywhere) 2) we start filling the decl sequence, and then encounter an expression type we do not know how to deal with.
Sign in to reply to this message.
Sorry if I missed the updated tests last time... i blame rietveld, although i've no idea if its just my fault. Anyways, feel free to submit this whenever, the nitpicks about the representation shouldn't hold anything up. http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp File lib/Analysis/ThreadSafety.cpp (right): http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:175: return; On 2011/09/14 01:41:57, supertri wrote: > On 2011/09/13 23:16:20, chandlerc wrote: > > Why bother to return or have an else? > > > > Alternatively make 171 read 'else if (isa<...>(Exp) && Parent)' > > Note that this would have different behaviour, because it would result in Valid > being set to false. In particular, when analyzing inline functions, the this > expressions are compared without parents. I'm still not seeing how the logic you have here is different... But its not a huge deal. http://codereview.appspot.com/4960064/diff/8001/lib/Analysis/ThreadSafety.cpp... lib/Analysis/ThreadSafety.cpp:185: if (DeclSeq.empty()) On 2011/09/14 01:41:57, supertri wrote: > On 2011/09/13 23:16:20, chandlerc wrote: > > I'm still a bit surprised we can't just impliment isValid() as > 'DeclSeq.empty()' > > and omit the bool entirely. > > There are two causes of unparseable lock expressions: > > 1) empty decl sequence (recursion bottomed out without going anywhere) > 2) we start filling the decl sequence, and then encounter an expression type we > do not know how to deal with. Sure, why not DeclSeq.clear() when #2 happens?
Sign in to reply to this message.
|
