<div class="gmail_quote">On Fri, Sep 9, 2011 at 9:04 AM, Caitlin Sadowski <span dir="ltr"><<a href="mailto:supertri@google.com">supertri@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Author: supertri<br>
Date: Fri Sep 9 11:04:02 2011<br>
New Revision: 139367<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=139367&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=139367&view=rev</a><br>
Log:<br>
Thread safety: refactoring to use an error handler<br>
<br>
Modified:<br>
cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
<br>
Modified: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=139367&r1=139366&r2=139367&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp?rev=139367&r1=139366&r2=139367&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp (original)<br>
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp Fri Sep 9 11:04:02 2011<br>
@@ -17,6 +17,7 @@<br>
#include "clang/Sema/SemaInternal.h"<br>
#include "clang/Sema/ScopeInfo.h"<br>
#include "clang/Basic/SourceManager.h"<br>
+#include "clang/Basic/SourceLocation.h"<br>
#include "clang/Lex/Preprocessor.h"<br>
#include "clang/AST/DeclObjC.h"<br>
#include "clang/AST/DeclCXX.h"<br>
@@ -37,6 +38,7 @@<br>
#include "llvm/ADT/ImmutableMap.h"<br>
#include "llvm/ADT/PostOrderIterator.h"<br>
#include "llvm/ADT/SmallVector.h"<br>
+#include "llvm/ADT/StringRef.h"<br>
#include "llvm/Support/Casting.h"<br>
#include <algorithm><br>
#include <vector><br>
@@ -603,6 +605,178 @@<br>
//===----------------------------------------------------------------------===//<br>
// -Wthread-safety<br>
//===----------------------------------------------------------------------===//<br>
+namespace clang {<br>
+namespace thread_safety {<br>
+typedef std::pair<SourceLocation, PartialDiagnostic> DelayedDiag;<br>
+typedef llvm::SmallVector<DelayedDiag, 4> DiagList;<br>
+<br>
+enum ProtectedOperationKind {<br>
+ POK_VarDereference,<br>
+ POK_VarAccess,<br>
+ POK_FunctionCall<br>
+};<br>
+<br>
+enum LockKind {<br>
+ LK_Shared,<br>
+ LK_Exclusive<br>
+};<br>
+<br>
+enum AccessKind {<br>
+ AK_Read,<br>
+ AK_Written<br>
+};<br>
+<br>
+<br>
+struct SortDiagBySourceLocation {<br>
+ Sema &S;<br>
+ SortDiagBySourceLocation(Sema &S) : S(S) {}<br>
+<br>
+ bool operator()(const DelayedDiag &left, const DelayedDiag &right) {<br>
+ // Although this call will be slow, this is only called when outputting<br>
+ // multiple warnings.<br>
+ return S.getSourceManager().isBeforeInTranslationUnit(left.first,<br>
+ right.first);<br>
+ }<br>
+};<br>
+<br>
+/// \brief Helper function that returns a LockKind required for the given level<br>
+/// of access.<br>
+LockKind getLockKindFromAccessKind(AccessKind AK) {<br>
+ switch (AK) {<br>
+ case AK_Read :<br>
+ return LK_Shared;<br>
+ case AK_Written :<br>
+ return LK_Exclusive;<br>
+ }<br>
+}<br>
+<br>
+class ThreadSafetyHandler {<br>
+public:<br>
+ typedef llvm::StringRef Name;<br>
+ ThreadSafetyHandler() {}<br>
+ virtual ~ThreadSafetyHandler() {}<br>
+ virtual void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {}<br>
+ virtual void handleDoubleLock(Name LockName, SourceLocation Loc) {}<br>
+ virtual void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){}<br>
+ virtual void handleNoLockLoopEntry(Name LockName, SourceLocation Loc) {}<br>
+ virtual void handleNoUnlock(Name LockName, Name FunName,<br>
+ SourceLocation Loc) {}<br>
+ virtual void handleExclusiveAndShared(Name LockName, SourceLocation Loc1,<br>
+ SourceLocation Loc2) {}<br>
+ virtual void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,<br>
+ AccessKind AK, SourceLocation Loc) {}<br>
+ virtual void handleMutexNotHeld(const NamedDecl *D,<br>
+ ProtectedOperationKind POK, Name LockName,<br>
+ LockKind LK, SourceLocation Loc) {}<br>
+ virtual void handleFunExcludesLock(Name FunName, Name LockName,<br>
+ SourceLocation Loc) {}<br>
+};<br>
+<br>
+class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {<br>
+ Sema &S;<br>
+ DiagList Warnings;<br>
+<br>
+ // Helper functions<br>
+ void warnLockMismatch(unsigned DiagID, Name LockName, SourceLocation Loc) {<br>
+ PartialDiagnostic Warning = S.PDiag(DiagID) << LockName;<br>
+ Warnings.push_back(DelayedDiag(Loc, Warning));<br>
+ }<br>
+<br>
+ public:<br>
+ ThreadSafetyReporter(Sema &S) : S(S) {}<br>
+<br>
+ /// \brief Emit all buffered diagnostics in order of sourcelocation.<br>
+ /// We need to output diagnostics produced while iterating through<br>
+ /// the lockset in deterministic order, so this function orders diagnostics<br>
+ /// and outputs them.<br>
+ void emitDiagnostics() {<br>
+ SortDiagBySourceLocation SortDiagBySL(S);<br>
+ sort(Warnings.begin(), Warnings.end(), SortDiagBySL);<br>
+ for (DiagList::iterator I = Warnings.begin(), E = Warnings.end();<br>
+ I != E; ++I)<br>
+ S.Diag(I->first, I->second);<br>
+ }<br>
+<br>
+ void handleUnmatchedUnlock(Name LockName, SourceLocation Loc) {<br>
+ warnLockMismatch(diag::warn_unlock_but_no_lock, LockName, Loc);<br>
+ }<br>
+<br>
+ void handleDoubleLock(Name LockName, SourceLocation Loc) {<br>
+ warnLockMismatch(diag::warn_double_lock, LockName, Loc);<br>
+ }<br>
+<br>
+ void handleMutexHeldEndOfScope(Name LockName, SourceLocation Loc){<br>
+ warnLockMismatch(diag::warn_lock_at_end_of_scope, LockName, Loc);<br>
+ }<br>
+<br>
+ void handleNoLockLoopEntry(Name LockName, SourceLocation Loc) {<br>
+ warnLockMismatch(diag::warn_expecting_lock_held_on_loop, LockName, Loc);<br>
+ }<br>
+<br>
+ void handleNoUnlock(Name LockName, llvm::StringRef FunName,<br>
+ SourceLocation Loc) {<br>
+ PartialDiagnostic Warning =<br>
+ S.PDiag(diag::warn_no_unlock) << LockName << FunName;<br>
+ Warnings.push_back(DelayedDiag(Loc, Warning));<br>
+ }<br>
+<br>
+ void handleExclusiveAndShared(Name LockName, SourceLocation Loc1,<br>
+ SourceLocation Loc2) {<br>
+ PartialDiagnostic Warning =<br>
+ S.PDiag(diag::warn_lock_exclusive_and_shared) << LockName;<br>
+ PartialDiagnostic Note =<br>
+ S.PDiag(diag::note_lock_exclusive_and_shared) << LockName;<br>
+ Warnings.push_back(DelayedDiag(Loc1, Warning));<br>
+ Warnings.push_back(DelayedDiag(Loc2, Note));<br>
+ }<br>
+<br>
+ void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,<br>
+ AccessKind AK, SourceLocation Loc) {<br>
+ unsigned DiagID;<br>
+ switch (POK) {<br>
+ case POK_VarAccess:<br>
+ DiagID = diag::warn_variable_requires_any_lock;<br>
+ break;<br>
+ case POK_VarDereference:<br>
+ DiagID = diag::warn_var_deref_requires_any_lock;<br>
+ break;<br>
+ default:<br>
+ return;<br>
+ break;<br>
+ }<br>
+ PartialDiagnostic Warning = S.PDiag(DiagID) << D->getName();<br>
+ Warnings.push_back(DelayedDiag(Loc, Warning));<br>
+ }<br>
+<br>
+ void handleMutexNotHeld(const NamedDecl *D, ProtectedOperationKind POK,<br>
+ Name LockName, LockKind LK, SourceLocation Loc) {<br>
+ unsigned DiagID;<br>
+ switch (POK) {<br>
+ case POK_VarAccess:<br>
+ DiagID = diag::warn_variable_requires_lock;<br>
+ break;<br>
+ case POK_VarDereference:<br>
+ DiagID = diag::warn_var_deref_requires_lock;<br>
+ break;<br>
+ case POK_FunctionCall:<br>
+ DiagID = diag::warn_fun_requires_lock;<br>
+ break;<br>
+ }<br>
+ PartialDiagnostic Warning = S.PDiag(DiagID)<br>
+ << D->getName().str() << LockName << LK;<br>
+ Warnings.push_back(DelayedDiag(Loc, Warning));<br>
+ }<br>
+<br>
+ void handleFunExcludesLock(Name FunName, Name LockName, SourceLocation Loc) {<br>
+ PartialDiagnostic Warning =<br>
+ S.PDiag(diag::warn_fun_excludes_mutex) << FunName << LockName;<br>
+ Warnings.push_back(DelayedDiag(Loc, Warning));<br>
+ }<br>
+};<br>
+}<br>
+}<br>
+<br>
+using namespace thread_safety;<br></blockquote><div><br></div><div>Is this necessary? I would generally prefer not to have a proliferation of using namespace declarations. This namespace in particular seems not unlikely to have names that collide with others in use in this file (that is, other namespaces within Analysis).</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
namespace {<br>
/// \brief Implements a set of CFGBlocks using a BitVector.<br>
@@ -713,7 +887,7 @@<br>
/// foo(MyL); // requires lock MyL->Mu to be held<br>
class MutexID {<br>
SmallVector<NamedDecl*, 2> DeclSeq;<br>
-<br>
+<br>
/// Build a Decl sequence representing the lock from the given expression.<br>
/// Recursive function that bottoms out when the final DeclRefExpr is reached.<br>
void buildMutexID(Expr *Exp) {<br>
@@ -772,16 +946,6 @@<br>
}<br>
};<br>
<br>
-enum LockKind {<br>
- LK_Shared,<br>
- LK_Exclusive<br>
-};<br>
-<br>
-enum AccessKind {<br>
- AK_Read,<br>
- AK_Written<br>
-};<br>
-<br>
/// \brief This is a helper class that stores info about the most recent<br>
/// accquire of a Lock.<br>
///<br>
@@ -824,7 +988,7 @@<br>
/// output error messages related to missing locks.<br>
/// FIXME: In future, we may be able to not inherit from a visitor.<br>
class BuildLockset : public StmtVisitor<BuildLockset> {<br>
- Sema &S;<br>
+ ThreadSafetyHandler &Handler;<br>
Lockset LSet;<br>
Lockset::Factory &LocksetFactory;<br>
<br>
@@ -833,7 +997,7 @@<br>
void addLock(SourceLocation LockLoc, Expr *LockExp, LockKind LK);<br>
const ValueDecl *getValueDecl(Expr *Exp);<br>
void warnIfMutexNotHeld (const NamedDecl *D, Expr *Exp, AccessKind AK,<br>
- MutexID &Mutex, unsigned DiagID);<br>
+ Expr *MutexExp, ProtectedOperationKind POK);<br>
void checkAccess(Expr *Exp, AccessKind AK);<br>
void checkDereference(Expr *Exp, AccessKind AK);<br>
<br>
@@ -842,7 +1006,7 @@<br>
<br>
/// \brief Returns true if the lockset contains a lock, regardless of whether<br>
/// the lock is held exclusively or shared.<br>
- bool locksetContains(MutexID Lock) {<br>
+ bool locksetContains(MutexID Lock) const {<br>
return LSet.lookup(Lock);<br>
}<br>
<br>
@@ -853,9 +1017,22 @@<br>
return (LockHeld && KindRequested == LockHeld->LKind);<br>
}<br>
<br>
+ /// \brief Returns true if the lockset contains a lock with at least the<br>
+ /// passed in locktype. So for example, if we pass in LK_Shared, this function<br>
+ /// returns true if the lock is held LK_Shared or LK_Exclusive. If we pass in<br>
+ /// LK_Exclusive, this function returns true if the lock is held LK_Exclusive.<br>
+ bool locksetContainsAtLeast(MutexID Lock, LockKind KindRequested) const {<br>
+ switch (KindRequested) {<br>
+ case LK_Shared:<br>
+ return locksetContains(Lock);<br>
+ case LK_Exclusive:<br>
+ return locksetContains(Lock, KindRequested);<br>
+ }<br>
+ }<br>
+<br>
public:<br>
- BuildLockset(Sema &S, Lockset LS, Lockset::Factory &F)<br>
- : StmtVisitor<BuildLockset>(), S(S), LSet(LS),<br>
+ BuildLockset(ThreadSafetyHandler &Handler, Lockset LS, Lockset::Factory &F)<br>
+ : StmtVisitor<BuildLockset>(), Handler(Handler), LSet(LS),<br>
LocksetFactory(F) {}<br>
<br>
Lockset getLockset() {<br>
@@ -879,7 +1056,7 @@<br>
<br>
// FIXME: Don't always warn when we have support for reentrant locks.<br>
if (locksetContains(Mutex))<br>
- S.Diag(LockLoc, diag::warn_double_lock) << Mutex.getName();<br>
+ Handler.handleDoubleLock(Mutex.getName(), LockLoc);<br>
LSet = LocksetFactory.add(LSet, Mutex, NewLock);<br>
}<br>
<br>
@@ -891,7 +1068,7 @@<br>
<br>
Lockset NewLSet = LocksetFactory.remove(LSet, Mutex);<br>
if(NewLSet == LSet)<br>
- S.Diag(UnlockLoc, diag::warn_unlock_but_no_lock) << Mutex.getName();<br>
+ Handler.handleUnmatchedUnlock(Mutex.getName(), UnlockLoc);<br>
<br>
LSet = NewLSet;<br>
}<br>
@@ -910,20 +1087,12 @@<br>
/// \brief Warn if the LSet does not contain a lock sufficient to protect access<br>
/// of at least the passed in AccessType.<br>
void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, Expr *Exp,<br>
- AccessKind AK, MutexID &Mutex,<br>
- unsigned DiagID) {<br>
- switch (AK) {<br>
- case AK_Read:<br>
- if (!locksetContains(Mutex))<br>
- S.Diag(Exp->getExprLoc(), DiagID)<br>
- << D->getName() << Mutex.getName() << LK_Shared;<br>
- break;<br>
- case AK_Written :<br>
- if (!locksetContains(Mutex, LK_Exclusive))<br>
- S.Diag(Exp->getExprLoc(), DiagID)<br>
- << D->getName() << Mutex.getName() << LK_Exclusive;<br>
- break;<br>
- }<br>
+ AccessKind AK, Expr *MutexExp,<br>
+ ProtectedOperationKind POK) {<br>
+ LockKind LK = getLockKindFromAccessKind(AK);<br>
+ MutexID Mutex(MutexExp);<br>
+ if (!locksetContainsAtLeast(Mutex, LK))<br>
+ Handler.handleMutexNotHeld(D, POK, Mutex.getName(), LK, Exp->getExprLoc());<br>
}<br>
<br>
<br>
@@ -944,17 +1113,12 @@<br>
return;<br>
<br>
if (D->getAttr<PtGuardedVarAttr>() && LSet.isEmpty())<br>
- S.Diag(Exp->getExprLoc(), diag::warn_var_deref_requires_any_lock)<br>
- << D->getName();<br>
+ Handler.handleNoMutexHeld(D, POK_VarDereference, AK, Exp->getExprLoc());<br>
<br>
const AttrVec &ArgAttrs = D->getAttrs();<br>
- for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) {<br>
- if (ArgAttrs[i]->getKind() != attr::PtGuardedBy)<br>
- continue;<br>
- const PtGuardedByAttr *PGBAttr = cast<PtGuardedByAttr>(ArgAttrs[i]);<br>
- MutexID Mutex(PGBAttr->getArg());<br>
- warnIfMutexNotHeld(D, Exp, AK, Mutex, diag::warn_var_deref_requires_lock);<br>
- }<br>
+ for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i)<br>
+ if (PtGuardedByAttr *PGBAttr = dyn_cast<PtGuardedByAttr>(ArgAttrs[i]))<br>
+ warnIfMutexNotHeld(D, Exp, AK, PGBAttr->getArg(), POK_VarDereference);<br>
}<br>
<br>
/// \brief Checks guarded_by and guarded_var attributes.<br>
@@ -967,17 +1131,12 @@<br>
return;<br>
<br>
if (D->getAttr<GuardedVarAttr>() && LSet.isEmpty())<br>
- S.Diag(Exp->getExprLoc(), diag::warn_variable_requires_any_lock)<br>
- << D->getName();<br>
+ Handler.handleNoMutexHeld(D, POK_VarAccess, AK, Exp->getExprLoc());<br>
<br>
const AttrVec &ArgAttrs = D->getAttrs();<br>
- for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i) {<br>
- if (ArgAttrs[i]->getKind() != attr::GuardedBy)<br>
- continue;<br>
- const GuardedByAttr *GBAttr = cast<GuardedByAttr>(ArgAttrs[i]);<br>
- MutexID Mutex(GBAttr->getArg());<br>
- warnIfMutexNotHeld(D, Exp, AK, Mutex, diag::warn_variable_requires_lock);<br>
- }<br>
+ for(unsigned i = 0, Size = ArgAttrs.size(); i < Size; ++i)<br>
+ if (GuardedByAttr *GBAttr = dyn_cast<GuardedByAttr>(ArgAttrs[i]))<br>
+ warnIfMutexNotHeld(D, Exp, AK, GBAttr->getArg(), POK_VarAccess);<br>
}<br>
<br>
/// \brief For unary operations which read and write a variable, we need to<br>
@@ -1048,7 +1207,7 @@<br>
///<br>
/// FIXME: For classes annotated with one of the guarded annotations, we need<br>
/// to treat const method calls as reads and non-const method calls as writes,<br>
-/// and check that the appropriate locks are held. Non-const method calls with<br>
+/// and check that the appropriate locks are held. Non-const method calls with<br>
/// the same signature as const method calls can be also treated as reads.<br>
///<br>
/// FIXME: We need to also visit CallExprs to catch/check global functions.<br>
@@ -1101,11 +1260,8 @@<br>
cast<ExclusiveLocksRequiredAttr>(Attr);<br>
<br>
for (ExclusiveLocksRequiredAttr::args_iterator<br>
- I = ELRAttr->args_begin(), E = ELRAttr->args_end(); I != E; ++I) {<br>
- MutexID Mutex(*I);<br>
- warnIfMutexNotHeld(D, Exp, AK_Written, Mutex,<br>
- diag::warn_fun_requires_lock);<br>
- }<br>
+ I = ELRAttr->args_begin(), E = ELRAttr->args_end(); I != E; ++I)<br>
+ warnIfMutexNotHeld(D, Exp, AK_Written, *I, POK_FunctionCall);<br>
break;<br>
}<br>
<br>
@@ -1116,11 +1272,8 @@<br>
SharedLocksRequiredAttr *SLRAttr = cast<SharedLocksRequiredAttr>(Attr);<br>
<br>
for (SharedLocksRequiredAttr::args_iterator I = SLRAttr->args_begin(),<br>
- E = SLRAttr->args_end(); I != E; ++I) {<br>
- MutexID Mutex(*I);<br>
- warnIfMutexNotHeld(D, Exp, AK_Read, Mutex,<br>
- diag::warn_fun_requires_lock);<br>
- }<br>
+ E = SLRAttr->args_end(); I != E; ++I)<br>
+ warnIfMutexNotHeld(D, Exp, AK_Read, *I, POK_FunctionCall);<br>
break;<br>
}<br>
<br>
@@ -1130,8 +1283,8 @@<br>
E = LEAttr->args_end(); I != E; ++I) {<br>
MutexID Mutex(*I);<br>
if (locksetContains(Mutex))<br>
- S.Diag(ExpLocation, diag::warn_fun_excludes_mutex)<br>
- << D->getName() << Mutex.getName();<br>
+ Handler.handleFunExcludesLock(D->getName(), Mutex.getName(),<br>
+ ExpLocation);<br>
}<br>
break;<br>
}<br>
@@ -1147,37 +1300,13 @@<br>
}<br>
}<br>
<br>
-typedef std::pair<SourceLocation, PartialDiagnostic> DelayedDiag;<br>
-typedef llvm::SmallVector<DelayedDiag, 4> DiagList;<br>
-<br>
-struct SortDiagBySourceLocation {<br>
- Sema &S;<br>
-<br>
- SortDiagBySourceLocation(Sema &S) : S(S) {}<br>
-<br>
- bool operator()(const DelayedDiag &left, const DelayedDiag &right) {<br>
- // Although this call will be slow, this is only called when outputting<br>
- // multiple warnings.<br>
- return S.getSourceManager().isBeforeInTranslationUnit(left.first,<br>
- right.first);<br>
- }<br>
-};<br>
} // end anonymous namespace<br>
<br>
-/// \brief Emit all buffered diagnostics in order of sourcelocation.<br>
-/// We need to output diagnostics produced while iterating through<br>
-/// the lockset in deterministic order, so this function orders diagnostics<br>
-/// and outputs them.<br>
-static void EmitDiagnostics(Sema &S, DiagList &D) {<br>
- SortDiagBySourceLocation SortDiagBySL(S);<br>
- sort(D.begin(), D.end(), SortDiagBySL);<br>
- for (DiagList::iterator I = D.begin(), E = D.end(); I != E; ++I)<br>
- S.Diag(I->first, I->second);<br>
-}<br>
-<br>
-static Lockset warnIfNotInFirstSetOrNotSameKind(Sema &S, const Lockset LSet1,<br>
+/// \brief Flags a warning for each lock that is in LSet2 but not LSet1, or<br>
+/// else mutexes that are held shared in one lockset and exclusive in the other.<br>
+static Lockset warnIfNotInFirstSetOrNotSameKind(ThreadSafetyHandler &Handler,<br>
+ const Lockset LSet1,<br>
const Lockset LSet2,<br>
- DiagList &Warnings,<br>
Lockset Intersection,<br>
Lockset::Factory &Fact) {<br>
for (Lockset::iterator I = LSet2.begin(), E = LSet2.end(); I != E; ++I) {<br>
@@ -1185,19 +1314,15 @@<br>
const LockData &LSet2LockData = I.getData();<br>
if (const LockData *LD = LSet1.lookup(LSet2Mutex)) {<br>
if (LD->LKind != LSet2LockData.LKind) {<br>
- PartialDiagnostic Warning =<br>
- S.PDiag(diag::warn_lock_exclusive_and_shared) << LSet2Mutex.getName();<br>
- PartialDiagnostic Note =<br>
- S.PDiag(diag::note_lock_exclusive_and_shared) << LSet2Mutex.getName();<br>
- Warnings.push_back(DelayedDiag(LSet2LockData.AcquireLoc, Warning));<br>
- Warnings.push_back(DelayedDiag(LD->AcquireLoc, Note));<br>
+ Handler.handleExclusiveAndShared(LSet2Mutex.getName(),<br>
+ LSet2LockData.AcquireLoc,<br>
+ LD->AcquireLoc);<br>
if (LD->LKind != LK_Exclusive)<br>
Intersection = Fact.add(Intersection, LSet2Mutex, LSet2LockData);<br>
}<br>
} else {<br>
- PartialDiagnostic Warning =<br>
- S.PDiag(diag::warn_lock_at_end_of_scope) << LSet2Mutex.getName();<br>
- Warnings.push_back(DelayedDiag(LSet2LockData.AcquireLoc, Warning));<br>
+ Handler.handleMutexHeldEndOfScope(LSet2Mutex.getName(),<br>
+ LSet2LockData.AcquireLoc);<br>
}<br>
}<br>
return Intersection;<br>
@@ -1212,27 +1337,22 @@<br>
/// A; if () then B; else C; D; we need to check that the lockset after B and C<br>
/// are the same. In the event of a difference, we use the intersection of these<br>
/// two locksets at the start of D.<br>
-static Lockset intersectAndWarn(Sema &S, const Lockset LSet1,<br>
- const Lockset LSet2,<br>
+static Lockset intersectAndWarn(ThreadSafetyHandler &Handler,<br>
+ const Lockset LSet1, const Lockset LSet2,<br>
Lockset::Factory &Fact) {<br>
Lockset Intersection = LSet1;<br>
- DiagList Warnings;<br>
-<br>
- Intersection = warnIfNotInFirstSetOrNotSameKind(S, LSet1, LSet2, Warnings,<br>
+ Intersection = warnIfNotInFirstSetOrNotSameKind(Handler, LSet1, LSet2,<br>
Intersection, Fact);<br>
<br>
for (Lockset::iterator I = LSet1.begin(), E = LSet1.end(); I != E; ++I) {<br>
if (!LSet2.contains(I.getKey())) {<br>
const MutexID &Mutex = I.getKey();<br>
const LockData &MissingLock = I.getData();<br>
- PartialDiagnostic Warning =<br>
- S.PDiag(diag::warn_lock_at_end_of_scope) << Mutex.getName();<br>
- Warnings.push_back(DelayedDiag(MissingLock.AcquireLoc, Warning));<br>
+ Handler.handleMutexHeldEndOfScope(Mutex.getName(),<br>
+ MissingLock.AcquireLoc);<br>
Intersection = Fact.remove(Intersection, Mutex);<br>
}<br>
}<br>
-<br>
- EmitDiagnostics(S, Warnings);<br>
return Intersection;<br>
}<br>
<br>
@@ -1260,38 +1380,35 @@<br>
///<br>
/// \param LoopEntrySet Locks before starting the loop<br>
/// \param LoopReentrySet Locks in the last CFG block of the loop<br>
-static void warnBackEdgeUnequalLocksets(Sema &S, const Lockset LoopReentrySet,<br>
+static void warnBackEdgeUnequalLocksets(ThreadSafetyHandler &Handler,<br>
+ const Lockset LoopReentrySet,<br>
const Lockset LoopEntrySet,<br>
SourceLocation FirstLocInLoop,<br>
Lockset::Factory &Fact) {<br>
assert(FirstLocInLoop.isValid());<br>
- DiagList Warnings;<br>
-<br>
// Warn for locks held at the start of the loop, but not the end.<br>
for (Lockset::iterator I = LoopEntrySet.begin(), E = LoopEntrySet.end();<br>
I != E; ++I) {<br>
if (!LoopReentrySet.contains(I.getKey())) {<br>
- const MutexID &Mutex = I.getKey();<br>
// We report this error at the location of the first statement in a loop<br>
- PartialDiagnostic Warning =<br>
- S.PDiag(diag::warn_expecting_lock_held_on_loop) << Mutex.getName();<br>
- Warnings.push_back(DelayedDiag(FirstLocInLoop, Warning));<br>
+ Handler.handleNoLockLoopEntry(I.getKey().getName(), FirstLocInLoop);<br>
}<br>
}<br>
<br>
// Warn for locks held at the end of the loop, but not at the start.<br>
- warnIfNotInFirstSetOrNotSameKind(S, LoopEntrySet, LoopReentrySet, Warnings,<br>
+ warnIfNotInFirstSetOrNotSameKind(Handler, LoopEntrySet, LoopReentrySet,<br>
LoopReentrySet, Fact);<br>
-<br>
- EmitDiagnostics(S, Warnings);<br>
}<br>
<br>
+<br>
+namespace clang { namespace thread_safety {<br></blockquote><div><br></div><div>This seems a bit unconventional. I would just use two lines.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
/// \brief Check a function's CFG for thread-safety violations.<br>
///<br>
/// We traverse the blocks in the CFG, compute the set of mutexes that are held<br>
/// at the end of each block, and issue warnings for thread safety violations.<br>
/// Each block in the CFG is traversed exactly once.<br>
-static void checkThreadSafety(Sema &S, AnalysisContext &AC) {<br>
+void runThreadSafetyAnalysis(AnalysisContext &AC,<br>
+ ThreadSafetyHandler &Handler) {<br>
CFG *CFGraph = AC.getCFG();<br>
if (!CFGraph) return;<br>
const Decl *D = AC.getDecl();<br>
@@ -1348,12 +1465,12 @@<br>
Entryset = ExitLocksets[PrevBlockID];<br>
LocksetInitialized = true;<br>
} else {<br>
- Entryset = intersectAndWarn(S, Entryset, ExitLocksets[PrevBlockID],<br>
- LocksetFactory);<br>
+ Entryset = intersectAndWarn(Handler, Entryset,<br>
+ ExitLocksets[PrevBlockID], LocksetFactory);<br>
}<br>
}<br>
<br>
- BuildLockset LocksetBuilder(S, Entryset, LocksetFactory);<br>
+ BuildLockset LocksetBuilder(Handler, Entryset, LocksetFactory);<br>
for (CFGBlock::const_iterator BI = CurrBlock->begin(),<br>
BE = CurrBlock->end(); BI != BE; ++BI) {<br>
if (const CFGStmt *CfgStmt = dyn_cast<CFGStmt>(&*BI))<br>
@@ -1383,14 +1500,13 @@<br>
<br>
Lockset PreLoop = EntryLocksets[FirstLoopBlock->getBlockID()];<br>
Lockset LoopEnd = ExitLocksets[CurrBlockID];<br>
- warnBackEdgeUnequalLocksets(S, LoopEnd, PreLoop, FirstLoopLocation,<br>
+ warnBackEdgeUnequalLocksets(Handler, LoopEnd, PreLoop, FirstLoopLocation,<br>
LocksetFactory);<br>
}<br>
}<br>
<br>
Lockset FinalLockset = ExitLocksets[CFGraph->getExit().getBlockID()];<br>
if (!FinalLockset.isEmpty()) {<br>
- DiagList Warnings;<br>
for (Lockset::iterator I=FinalLockset.begin(), E=FinalLockset.end();<br>
I != E; ++I) {<br>
const MutexID &Mutex = I.getKey();<br>
@@ -1401,15 +1517,13 @@<br>
FunName = ContextDecl->getDeclName().getAsString();<br>
}<br>
<br>
- PartialDiagnostic Warning =<br>
- S.PDiag(diag::warn_no_unlock)<br>
- << Mutex.getName() << FunName;<br>
- Warnings.push_back(DelayedDiag(MissingLock.AcquireLoc, Warning));<br>
+ Handler.handleNoUnlock(Mutex.getName(), FunName, MissingLock.AcquireLoc);<br>
}<br>
- EmitDiagnostics(S, Warnings);<br>
}<br>
}<br>
<br>
+}} // end namespace clang::thread_safety<br>
+<br>
<br>
//===----------------------------------------------------------------------===//<br>
// AnalysisBasedWarnings - Worker object used by Sema to execute analysis-based<br>
@@ -1571,10 +1685,13 @@<br>
// Warning: check for unreachable code<br>
if (P.enableCheckUnreachable)<br>
CheckUnreachable(S, AC);<br>
-<br>
+<br>
// Check for thread safety violations<br>
- if (P.enableThreadSafetyAnalysis)<br>
- checkThreadSafety(S, AC);<br>
+ if (P.enableThreadSafetyAnalysis) {<br>
+ thread_safety::ThreadSafetyReporter Reporter(S);<br>
+ thread_safety::runThreadSafetyAnalysis(AC, Reporter);<br>
+ Reporter.emitDiagnostics();<br>
+ }<br>
<br>
if (Diags.getDiagnosticLevel(diag::warn_uninit_var, D->getLocStart())<br>
!= Diagnostic::Ignored ||<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br>