Issue 4960064: Adding support for unparseable locks

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 8 months ago by supertri

Modified:
12 years, 8 months ago

Reviewers:
chandlerc

Visibility:
Public.

Patch Set 2 : Cleaned up slightly #

Total comments: 6

Patch Set 3 : Incorporating chandler comments #

Patch Set 4 : Fixed version #

Total comments: 6

Created: 12 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+72 lines, -17 lines)			Patch
	M	lib/Analysis/ThreadSafety.cpp	View	1 2 3	9 chunks	+36 lines, -15 lines	6 comments	Download
	M	test/SemaCXX/warn-thread-safety-analysis.cpp	View	1 2 3	3 chunks	+36 lines, -2 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

chandlerc

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#oldcode159 lib/Analysis/ThreadSafety.cpp:159: Why the formatting change? (i find it makes the ...

12 years, 8 months ago (2011-09-12 04:34:20 UTC) #1

supertri

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): ...

12 years, 8 months ago (2011-09-12 17:15:25 UTC) #2

supertri

Actually, that one was hilariously broken. Here is a better version. Cheers, Caitlin On Mon, ...

12 years, 8 months ago (2011-09-13 17:34:52 UTC) #3

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}}
> +}
> +
>
>
>

chandlerc

Fundamentally, you still need to add tests that produce errors because of invalid locks. There ...

12 years, 8 months ago (2011-09-13 23:16:20 UTC) #4

supertri

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 ...

12 years, 8 months ago (2011-09-14 01:41:57 UTC) #5

chandlerc

12 years, 8 months ago (2011-09-14 03:01:43 UTC) #6

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?

Expand All Messages | Collapse All Messages