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