[cfe-commits] r163397 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/SemaDeclAttr.cpp test/SemaCXX/warn-thread-safety-analysis.cpp
Delesley Hutchins
delesley at google.com
Fri Sep 7 11:55:11 PDT 2012
Thank you for the fix; please accept my apology. I actually did build
and test before submit, but I did so with gcc, which accepts this code
without even a warning! Perhaps I should use a different C++
compiler... :-)
-DeLesley
On Fri, Sep 7, 2012 at 11:51 AM, Chad Rosier <michael_rosier at apple.com> wrote:
> Delesley,
> I addressed a few issues with this commit in r163403 and r163404. Please be more careful in the future. Particularly, make sure the compiler builds and passes all regression/lit tests before committing.
>
> Chad
>
> On Sep 7, 2012, at 1:34 PM, DeLesley Hutchins wrote:
>
>> Author: delesley
>> Date: Fri Sep 7 12:34:53 2012
>> New Revision: 163397
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=163397&view=rev
>> Log:
>> Thread-safety analysis: Add support for selectively turning off warnings
>> within part of a particular method.
>>
>> Modified:
>> cfe/trunk/lib/Analysis/ThreadSafety.cpp
>> cfe/trunk/lib/Sema/SemaDeclAttr.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=163397&r1=163396&r2=163397&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Analysis/ThreadSafety.cpp (original)
>> +++ cfe/trunk/lib/Analysis/ThreadSafety.cpp Fri Sep 7 12:34:53 2012
>> @@ -70,18 +70,19 @@
>> class SExpr {
>> private:
>> enum ExprOp {
>> - EOP_Nop, ///< No-op
>> - EOP_Wildcard, ///< Matches anything.
>> - EOP_This, ///< This keyword.
>> - EOP_NVar, ///< Named variable.
>> - EOP_LVar, ///< Local variable.
>> - EOP_Dot, ///< Field access
>> - EOP_Call, ///< Function call
>> - EOP_MCall, ///< Method call
>> - EOP_Index, ///< Array index
>> - EOP_Unary, ///< Unary operation
>> - EOP_Binary, ///< Binary operation
>> - EOP_Unknown ///< Catchall for everything else
>> + EOP_Nop, ///< No-op
>> + EOP_Wildcard, ///< Matches anything.
>> + EOP_Universal, ///< Universal lock.
>> + EOP_This, ///< This keyword.
>> + EOP_NVar, ///< Named variable.
>> + EOP_LVar, ///< Local variable.
>> + EOP_Dot, ///< Field access
>> + EOP_Call, ///< Function call
>> + EOP_MCall, ///< Method call
>> + EOP_Index, ///< Array index
>> + EOP_Unary, ///< Unary operation
>> + EOP_Binary, ///< Binary operation
>> + EOP_Unknown ///< Catchall for everything else
>> };
>>
>>
>> @@ -118,18 +119,19 @@
>>
>> unsigned arity() const {
>> switch (Op) {
>> - case EOP_Nop: return 0;
>> - case EOP_Wildcard: return 0;
>> - case EOP_NVar: return 0;
>> - case EOP_LVar: return 0;
>> - case EOP_This: return 0;
>> - case EOP_Dot: return 1;
>> - case EOP_Call: return Flags+1; // First arg is function.
>> - case EOP_MCall: return Flags+1; // First arg is implicit obj.
>> - case EOP_Index: return 2;
>> - case EOP_Unary: return 1;
>> - case EOP_Binary: return 2;
>> - case EOP_Unknown: return Flags;
>> + case EOP_Nop: return 0;
>> + case EOP_Wildcard: return 0;
>> + case EOP_Universal: return 0;
>> + case EOP_NVar: return 0;
>> + case EOP_LVar: return 0;
>> + case EOP_This: return 0;
>> + case EOP_Dot: return 1;
>> + case EOP_Call: return Flags+1; // First arg is function.
>> + case EOP_MCall: return Flags+1; // First arg is implicit obj.
>> + case EOP_Index: return 2;
>> + case EOP_Unary: return 1;
>> + case EOP_Binary: return 2;
>> + case EOP_Unknown: return Flags;
>> }
>> return 0;
>> }
>> @@ -194,6 +196,11 @@
>> return NodeVec.size()-1;
>> }
>>
>> + unsigned makeUniversal() {
>> + NodeVec.push_back(SExprNode(EOP_Universal, 0, 0));
>> + return NodeVec.size()-1;
>> + }
>> +
>> unsigned makeNamedVar(const NamedDecl *D) {
>> NodeVec.push_back(SExprNode(EOP_NVar, 0, D));
>> return NodeVec.size()-1;
>> @@ -447,10 +454,18 @@
>> void buildSExprFromExpr(Expr *MutexExp, Expr *DeclExp, const NamedDecl *D) {
>> CallingContext CallCtx(D);
>>
>> - // Ignore string literals
>> - if (MutexExp && isa<StringLiteral>(MutexExp)) {
>> - makeNop();
>> - return;
>> +
>> + if (MutexExp) {
>> + if (StringLiteral* SLit = dyn_cast<StringLiteral>(MutexExp)) {
>> + if (SLit->getString() == StringRef("*"))
>> + // The "*" expr is a universal lock, which essentially turns off
>> + // checks until it is removed from the lockset.
>> + makeUniversal();
>> + else
>> + // Ignore other string literals for now.
>> + makeNop();
>> + return;
>> + }
>> }
>>
>> // If we are processing a raw attribute expression, with no substitutions.
>> @@ -520,6 +535,11 @@
>> return NodeVec[0].kind() == EOP_Nop;
>> }
>>
>> + bool isUniversal() const {
>> + assert(NodeVec.size() > 0 && "Invalid Mutex");
>> + return NodeVec[0].kind() == EOP_Universal;
>> + }
>> +
>> /// Issue a warning about an invalid lock expression
>> static void warnInvalidLock(ThreadSafetyHandler &Handler, Expr* MutexExp,
>> Expr *DeclExp, const NamedDecl* D) {
>> @@ -567,6 +587,8 @@
>> return "_";
>> case EOP_Wildcard:
>> return "(?)";
>> + case EOP_Universal:
>> + return "*";
>> case EOP_This:
>> return "this";
>> case EOP_NVar:
>> @@ -709,6 +731,10 @@
>> ID.AddInteger(AcquireLoc.getRawEncoding());
>> ID.AddInteger(LKind);
>> }
>> +
>> + bool isAtLeast(LockKind LK) {
>> + return (LK == LK_Shared) || (LKind == LK_Exclusive);
>> + }
>> };
>>
>>
>> @@ -796,7 +822,16 @@
>>
>> LockData* findLock(FactManager& FM, const SExpr& M) const {
>> for (const_iterator I=begin(), E=end(); I != E; ++I) {
>> - if (FM[*I].MutID.matches(M)) return &FM[*I].LDat;
>> + const SExpr& E = FM[*I].MutID;
>> + if (E.matches(M)) return &FM[*I].LDat;
>> + }
>> + return 0;
>> + }
>> +
>> + LockData* findLockUniv(FactManager& FM, const SExpr& M) const {
>> + for (const_iterator I=begin(), E=end(); I != E; ++I) {
>> + const SExpr& E = FM[*I].MutID;
>> + if (E.matches(M) || E.isUniversal()) return &FM[*I].LDat;
>> }
>> return 0;
>> }
>> @@ -1654,39 +1689,12 @@
>>
>> void warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp, AccessKind AK,
>> Expr *MutexExp, ProtectedOperationKind POK);
>> + void warnIfMutexHeld(const NamedDecl *D, Expr *Exp, Expr *MutexExp);
>>
>> void checkAccess(Expr *Exp, AccessKind AK);
>> void checkDereference(Expr *Exp, AccessKind AK);
>> void handleCall(Expr *Exp, const NamedDecl *D, VarDecl *VD = 0);
>>
>> - /// \brief Returns true if the lockset contains a lock, regardless of whether
>> - /// the lock is held exclusively or shared.
>> - bool locksetContains(const SExpr &Mu) const {
>> - return FSet.findLock(Analyzer->FactMan, Mu);
>> - }
>> -
>> - /// \brief Returns true if the lockset contains a lock with the passed in
>> - /// locktype.
>> - bool locksetContains(const SExpr &Mu, LockKind KindRequested) const {
>> - const LockData *LockHeld = FSet.findLock(Analyzer->FactMan, Mu);
>> - return (LockHeld && KindRequested == LockHeld->LKind);
>> - }
>> -
>> - /// \brief Returns true if the lockset contains a lock with at least the
>> - /// passed in locktype. So for example, if we pass in LK_Shared, this function
>> - /// returns true if the lock is held LK_Shared or LK_Exclusive. If we pass in
>> - /// LK_Exclusive, this function returns true if the lock is held LK_Exclusive.
>> - bool locksetContainsAtLeast(const SExpr &Lock,
>> - LockKind KindRequested) const {
>> - switch (KindRequested) {
>> - case LK_Shared:
>> - return locksetContains(Lock);
>> - case LK_Exclusive:
>> - return locksetContains(Lock, KindRequested);
>> - }
>> - llvm_unreachable("Unknown LockKind");
>> - }
>> -
>> public:
>> BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
>> : StmtVisitor<BuildLockset>(),
>> @@ -1724,15 +1732,35 @@
>> LockKind LK = getLockKindFromAccessKind(AK);
>>
>> SExpr Mutex(MutexExp, Exp, D);
>> - if (!Mutex.isValid())
>> + if (!Mutex.isValid()) {
>> SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D);
>> - else if (Mutex.shouldIgnore())
>> - return; // A Nop is an invalid mutex that we've decided to ignore.
>> - else if (!locksetContainsAtLeast(Mutex, LK))
>> + return;
>> + } else if (Mutex.shouldIgnore()) {
>> + return;
>> + }
>> +
>> + LockData* LDat = FSet.findLockUniv(Analyzer->FactMan, Mutex);
>> + if (!LDat || !LDat->isAtLeast(LK))
>> Analyzer->Handler.handleMutexNotHeld(D, POK, Mutex.toString(), LK,
>> Exp->getExprLoc());
>> }
>>
>> +/// \brief Warn if the LSet contains the given lock.
>> +void BuildLockset::warnIfMutexHeld(const NamedDecl *D, Expr* Exp,
>> + Expr *MutexExp) {
>> + SExpr Mutex(MutexExp, Exp, D);
>> + if (!Mutex.isValid()) {
>> + SExpr::warnInvalidLock(Analyzer->Handler, MutexExp, Exp, D);
>> + return;
>> + }
>> +
>> + LockData* LDat = FSet.findLock(Analyzer->FactMan, Mutex);
>> + if (LDat)
>> + Analyzer->Handler.handleFunExcludesLock(D->getName(), Mutex.toString(),
>> + Exp->getExprLoc());
>> +}
>> +
>> +
>> /// \brief This method identifies variable dereferences and checks pt_guarded_by
>> /// and pt_guarded_var annotations. Note that we only check these annotations
>> /// at the time a pointer is dereferenced.
>> @@ -1841,15 +1869,10 @@
>>
>> case attr::LocksExcluded: {
>> LocksExcludedAttr *A = cast<LocksExcludedAttr>(At);
>> +
>> for (LocksExcludedAttr::args_iterator I = A->args_begin(),
>> E = A->args_end(); I != E; ++I) {
>> - SExpr Mutex(*I, Exp, D);
>> - if (!Mutex.isValid())
>> - SExpr::warnInvalidLock(Analyzer->Handler, *I, Exp, D);
>> - else if (locksetContains(Mutex))
>> - Analyzer->Handler.handleFunExcludesLock(D->getName(),
>> - Mutex.toString(),
>> - Exp->getExprLoc());
>> + warnIfMutexHeld(D, Exp, *I);
>> }
>> break;
>> }
>> @@ -2037,7 +2060,7 @@
>> JoinLoc, LEK1);
>> }
>> }
>> - else if (!LDat2.Managed)
>> + else if (!LDat2.Managed && !FSet2Mutex.isUniversal())
>> Handler.handleMutexHeldEndOfScope(FSet2Mutex.toString(),
>> LDat2.AcquireLoc,
>> JoinLoc, LEK1);
>> @@ -2060,7 +2083,7 @@
>> JoinLoc, LEK1);
>> }
>> }
>> - else if (!LDat1.Managed)
>> + else if (!LDat1.Managed && !FSet1Mutex.isUniversal())
>> Handler.handleMutexHeldEndOfScope(FSet1Mutex.toString(),
>> LDat1.AcquireLoc,
>> JoinLoc, LEK2);
>>
>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev=163397&r1=163396&r2=163397&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Fri Sep 7 12:34:53 2012
>> @@ -415,8 +415,10 @@
>> }
>>
>> if (StringLiteral *StrLit = dyn_cast<StringLiteral>(ArgExp)) {
>> - if (StrLit->getLength() == 0) {
>> + if (StrLit->getLength() == 0 ||
>> + StrLit->getString() == StringRef("*")) {
>> // Pass empty strings to the analyzer without warnings.
>> + // Treat "*" as the universal lock.
>> Args.push_back(ArgExp);
>> continue;
>> }
>>
>> 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=163397&r1=163396&r2=163397&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp Fri Sep 7 12:34:53 2012
>> @@ -24,10 +24,6 @@
>> __attribute__ ((shared_locks_required(__VA_ARGS__)))
>> #define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis))
>>
>> -//-----------------------------------------//
>> -// Helper fields
>> -//-----------------------------------------//
>> -
>>
>> class __attribute__((lockable)) Mutex {
>> public:
>> @@ -60,6 +56,14 @@
>> };
>>
>>
>> +// The universal lock, written "*", allows checking to be selectively turned
>> +// off for a particular piece of code.
>> +void beginNoWarnOnReads() SHARED_LOCK_FUNCTION("*");
>> +void endNoWarnOnReads() UNLOCK_FUNCTION("*");
>> +void beginNoWarnOnWrites() EXCLUSIVE_LOCK_FUNCTION("*");
>> +void endNoWarnOnWrites() UNLOCK_FUNCTION("*");
>> +
>> +
>> template<class T>
>> class SmartPtr {
>> public:
>> @@ -3217,3 +3221,79 @@
>> }
>>
>>
>> +namespace UniversalLock {
>> +
>> +class Foo {
>> + Mutex mu_;
>> + bool c;
>> +
>> + int a GUARDED_BY(mu_);
>> + void r_foo() SHARED_LOCKS_REQUIRED(mu_);
>> + void w_foo() EXCLUSIVE_LOCKS_REQUIRED(mu_);
>> +
>> + void test1() {
>> + int b;
>> +
>> + beginNoWarnOnReads();
>> + b = a;
>> + r_foo();
>> + endNoWarnOnReads();
>> +
>> + beginNoWarnOnWrites();
>> + a = 0;
>> + w_foo();
>> + endNoWarnOnWrites();
>> + }
>> +
>> + // don't warn on joins with universal lock
>> + void test2() {
>> + if (c) {
>> + beginNoWarnOnWrites();
>> + }
>> + a = 0; // \
>> + // expected-warning {{writing variable 'a' requires locking 'mu_' exclusively}}
>> + endNoWarnOnWrites(); // \
>> + // expected-warning {{unlocking '*' that was not locked}}
>> + }
>> +
>> +
>> + // make sure the universal lock joins properly
>> + void test3() {
>> + if (c) {
>> + mu_.Lock();
>> + beginNoWarnOnWrites();
>> + }
>> + else {
>> + beginNoWarnOnWrites();
>> + mu_.Lock();
>> + }
>> + a = 0;
>> + endNoWarnOnWrites();
>> + mu_.Unlock();
>> + }
>> +
>> +
>> + // combine universal lock with other locks
>> + void test4() {
>> + beginNoWarnOnWrites();
>> + mu_.Lock();
>> + mu_.Unlock();
>> + endNoWarnOnWrites();
>> +
>> + mu_.Lock();
>> + beginNoWarnOnWrites();
>> + endNoWarnOnWrites();
>> + mu_.Unlock();
>> +
>> + mu_.Lock();
>> + beginNoWarnOnWrites();
>> + mu_.Unlock();
>> + endNoWarnOnWrites();
>> + }
>> +};
>> +
>> +}
>> +
>> +
>> +
>> +
>>
>>
>> _______________________________________________
>> 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