[cfe-commits] r163397 - in /cfe/trunk: lib/Analysis/ThreadSafety.cpp lib/Sema/SemaDeclAttr.cpp test/SemaCXX/warn-thread-safety-analysis.cpp
Chandler Carruth
chandlerc at google.com
Fri Sep 7 13:00:36 PDT 2012
On Fri, Sep 7, 2012 at 1:34 PM, DeLesley Hutchins <delesley at google.com>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.
>
This is a pretty big new feature in the thread safety annotations and
analysis. I think we should probably discuss it on cfe-dev and make sure
the design is right and there aren't any serious problems with the proposal.
Currently, I don't really understand the use cases that make this solution
(as opposed to other techniques for turning off thread safety analysis) the
best solution. I suspect others may have similar questions.
Also, whatever the final design for this is should get documented carefully
so that we have something to refer people to when using these types of
features.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120907/48173a13/attachment.html>
More information about the cfe-commits
mailing list