From: Caitlin Sadowski Date: Mon, 29 Aug 2011 17:12:27 +0000 (+0000) Subject: Thread safety: various minor bugfixes, with test cases X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b4d0a9678f8c592990593233e64c59247f40a74a;p=clang Thread safety: various minor bugfixes, with test cases This patch is by DeLesley Hutchins. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@138738 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 6c074188b3..830846df46 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -622,6 +622,12 @@ public: /// \brief Set the bit associated with a particular CFGBlock. /// This is the important method for the SetType template parameter. bool insert(const CFGBlock *Block) { + // Note that insert() is called by po_iterator, which doesn't check to make + // sure that Block is non-null. Moreover, the CFGBlock iterator will + // occasionally hand out null pointers for pruned edges, so we catch those + // here. + if (Block == 0) + return false; // if an edge is trivially false. if (VisitedBlockIDs.test(Block->getBlockID())) return false; VisitedBlockIDs.set(Block->getBlockID()); @@ -629,7 +635,8 @@ public: } /// \brief Check if the bit for a CFGBlock has been already set. - /// This mehtod is for tracking visited blocks in the main threadsafety loop. + /// This method is for tracking visited blocks in the main threadsafety loop. + /// Block must not be null. bool alreadySet(const CFGBlock *Block) { return VisitedBlockIDs.test(Block->getBlockID()); } @@ -855,7 +862,8 @@ void BuildLockset::VisitDeclRefExpr(DeclRefExpr *Exp) { /// the method that is being called and add, remove or check locks in the /// lockset accordingly. void BuildLockset::VisitCXXMemberCallExpr(CXXMemberCallExpr *Exp) { - NamedDecl *D = dyn_cast(Exp->getCalleeDecl()); + NamedDecl *D = dyn_cast_or_null(Exp->getCalleeDecl()); + SourceLocation ExpLocation = Exp->getExprLoc(); Expr *Parent = Exp->getImplicitObjectArgument(); @@ -975,12 +983,19 @@ static Lockset intersectAndWarn(Sema &S, Lockset LSet1, Lockset LSet2, /// \brief Returns the location of the first Stmt in a Block. static SourceLocation getFirstStmtLocation(CFGBlock *Block) { + SourceLocation Loc; for (CFGBlock::const_iterator BI = Block->begin(), BE = Block->end(); BI != BE; ++BI) { - if (const CFGStmt *CfgStmt = dyn_cast(&(*BI))) - return CfgStmt->getStmt()->getLocStart(); + if (const CFGStmt *CfgStmt = dyn_cast(&(*BI))) { + Loc = CfgStmt->getStmt()->getLocStart(); + if (Loc.isValid()) return Loc; + } + } + if (Stmt *S = Block->getTerminator().getStmt()) { + Loc = S->getLocStart(); + if (Loc.isValid()) return Loc; } - return SourceLocation(); + return Loc; } /// \brief Warn about different locksets along backedges of loops. @@ -1033,10 +1048,6 @@ static void checkThreadSafety(Sema &S, AnalysisContext &AC) { CFG *CFGraph = AC.getCFG(); if (!CFGraph) return; - StringRef FunName; - if (const NamedDecl *ContextDecl = dyn_cast(AC.getDecl())) - FunName = ContextDecl->getName(); - Lockset::Factory LocksetFactory; // FIXME: Swith to SmallVector? Otherwise improve performance impact? @@ -1080,7 +1091,7 @@ static void checkThreadSafety(Sema &S, AnalysisContext &AC) { PE = CurrBlock->pred_end(); PI != PE; ++PI) { // if *PI -> CurrBlock is a back edge - if (!VisitedBlocks.alreadySet(*PI)) + if (*PI == 0 || !VisitedBlocks.alreadySet(*PI)) continue; int PrevBlockID = (*PI)->getBlockID(); @@ -1096,9 +1107,8 @@ static void checkThreadSafety(Sema &S, AnalysisContext &AC) { BuildLockset LocksetBuilder(S, Entryset, LocksetFactory); for (CFGBlock::const_iterator BI = CurrBlock->begin(), BE = CurrBlock->end(); BI != BE; ++BI) { - if (const CFGStmt *CfgStmt = dyn_cast(&*BI)) { + if (const CFGStmt *CfgStmt = dyn_cast(&*BI)) LocksetBuilder.Visit(const_cast(CfgStmt->getStmt())); - } } Exitset = LocksetBuilder.getLockset(); @@ -1110,12 +1120,17 @@ static void checkThreadSafety(Sema &S, AnalysisContext &AC) { SE = CurrBlock->succ_end(); SI != SE; ++SI) { // if CurrBlock -> *SI is *not* a back edge - if (!VisitedBlocks.alreadySet(*SI)) + if (*SI == 0 || !VisitedBlocks.alreadySet(*SI)) continue; CFGBlock *FirstLoopBlock = *SI; SourceLocation FirstLoopLocation = getFirstStmtLocation(FirstLoopBlock); + assert(FirstLoopLocation.isValid()); + // Fail gracefully in release code. + if (!FirstLoopLocation.isValid()) + continue; + Lockset PreLoop = EntryLocksets[FirstLoopBlock->getBlockID()]; Lockset LoopEnd = ExitLocksets[CurrBlockID]; warnBackEdgeUnequalLocksets(S, LoopEnd, PreLoop, FirstLoopLocation); @@ -1129,6 +1144,12 @@ static void checkThreadSafety(Sema &S, AnalysisContext &AC) { I != E; ++I) { const LockID &MissingLock = I.getKey(); const LockData &MissingLockData = I.getData(); + + std::string FunName = ""; + if (const NamedDecl *ContextDecl = dyn_cast(AC.getDecl())) { + FunName = ContextDecl->getDeclName().getAsString(); + } + PartialDiagnostic Warning = S.PDiag(diag::warn_locks_not_released) << MissingLock.getName() << FunName; diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index e8b7e9fb15..831c26ca3b 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -265,3 +265,33 @@ void aa_fun_bad_3() { glock.globalLock(); // \ expected-warning {{lock 'aa_mu' is not released at the end of function 'aa_fun_bad_3'}} } + + +//--------------------------------------------------// +// Regression tests for unusual method names +//--------------------------------------------------// + +Mutex wmu; + +// Test diagnostics for other method names. +class WeirdMethods { + WeirdMethods() { + wmu.Lock(); // \ + expected-warning {{lock 'wmu' is not released at the end of function 'WeirdMethods'}} + } + ~WeirdMethods() { + wmu.Lock(); // \ + expected-warning {{lock 'wmu' is not released at the end of function '~WeirdMethods'}} + } + void operator++() { + wmu.Lock(); // \ + expected-warning {{lock 'wmu' is not released at the end of function 'operator++'}} + } + operator int*() { + wmu.Lock(); // \ + expected-warning {{lock 'wmu' is not released at the end of function 'operator int *'}} + return 0; + } +}; + + diff --git a/test/SemaCXX/warn-thread-safety-parsing.cpp b/test/SemaCXX/warn-thread-safety-parsing.cpp index 04680c519b..e943f3b383 100644 --- a/test/SemaCXX/warn-thread-safety-parsing.cpp +++ b/test/SemaCXX/warn-thread-safety-parsing.cpp @@ -1139,3 +1139,61 @@ int slr_function_bad_3() __attribute__((shared_locks_required(muDoublePointer))) int slr_function_bad_4() __attribute__((shared_locks_required(umu))); // \ expected-error {{'shared_locks_required' attribute requires arguments whose type is annotated with 'lockable' attribute}} + +//-----------------------------------------// +// Regression tests for unusual cases. +//-----------------------------------------// + +int trivially_false_edges(bool b) { + // Create NULL (never taken) edges in CFG + if (false) return 1; + else return 2; +} + +// Possible Clang bug -- method pointer in template parameter +class UnFoo { +public: + void foo(); +}; + +template +class MCaller { +public: + static void call_method_ptr(UnFoo *f) { + // FIXME: Possible Clang bug: + // getCalleeDecl() returns NULL in the following case: + (f->*methptr)(); + } +}; + +void call_method_ptr_inst(UnFoo* f) { + MCaller<&UnFoo::foo>::call_method_ptr(f); +} + +int temp; +void empty_back_edge() { + // Create a back edge to a block with with no statements + for (;;) { + ++temp; + if (temp > 10) break; + } +} + +struct Foomger { + void operator++(); +}; + +struct Foomgoper { + Foomger f; + + bool done(); + void invalid_back_edge() { + do { + // FIXME: Possible Clang bug: + // The first statement in this basic block has no source location + ++f; + } while (!done()); + } +}; + +