[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:41:26 PDT 2012
On Fri, Sep 7, 2012 at 4:37 PM, Delesley Hutchins <delesley at google.com>wrote:
> 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.
But this is a very different mechanism. Just because the GCC system
supported something like this doesn't necessarily mean we should support it
in Clang. It is certainly a factor, but one that we should weigh in
conjunction with other factors. Anyways, I don't really want to have the
design discussion here. I think this really belongs on cfe-dev, and we
should move the discussion there rather than here. Your email actually
sounds like a pretty good starting point....
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120907/a35429fe/attachment.html>
More information about the cfe-commits
mailing list