[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 13:37:52 PDT 2012
This isn't actually all that big a feature, and it's one that is
implemented in gcc annotalysis. Thread safety analysis currently
supports NO_THREAD_SAFETY_ANALYSIS as an escape hatch for methods that
violate thread safety rules. However, it is often the case that you
don't want to turn off thread safety for an entire method, but just
for a particular racy line. For example:
class Foo {
Mutex mu_;
int data_ GUARDED_BY(mu_);
void foo();
};
void foo() {
mu_.Lock();
data_ = computeSomeData();
mu_.Unlock();
beginNoWarnOnRead();
Logfile << "The data is " << data_ << ".\n"; // Thread safety
turned off for this line only.
endNoWarnOnRead();
}
Here, you don't want to hold the lock while writing to a file, and you
don't care about race conditions in the log.
As far as implementation goes, the analysis already tracks pairs of
Lock()/Unlock() statements in the CFG, so it was natural to reuse the
existing implementation for beginNoWarn() and endNoWarn(); they
operate in the exact same way.
You are absolutely right about the documentation, though. What I
really need to do is put together a web page, with an example
thread_annotations.h file that shows how to use the attributes, so
that users don't have to search warn-thread-safety-analysis.cpp for
examples.
-DeLesley
On Fri, Sep 7, 2012 at 1:00 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 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
>
>
--
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315
More information about the cfe-commits
mailing list