Thread safety: various minor bugfixes, with test cases

This patch is by DeLesley Hutchins.

llvm-svn: 138738

clang/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<NamedDecl>(Exp->getCalleeDecl());
+  NamedDecl *D = dyn_cast_or_null<NamedDecl>(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<CFGStmt>(&(*BI)))
-      return CfgStmt->getStmt()->getLocStart();
+    if (const CFGStmt *CfgStmt = dyn_cast<CFGStmt>(&(*BI))) {
+      Loc = CfgStmt->getStmt()->getLocStart();
+      if (Loc.isValid()) return Loc;
+    }
   }
-  return SourceLocation();
+  if (Stmt *S = Block->getTerminator().getStmt()) {
+    Loc = S->getLocStart();
+    if (Loc.isValid()) return Loc;
+  }
+  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<NamedDecl>(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<CFGStmt>(&*BI)) {
+      if (const CFGStmt *CfgStmt = dyn_cast<CFGStmt>(&*BI))
         LocksetBuilder.Visit(const_cast<Stmt*>(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 = "<unknown>";
+      if (const NamedDecl *ContextDecl = dyn_cast<NamedDecl>(AC.getDecl())) {
+        FunName = ContextDecl->getDeclName().getAsString();
+      }
+
       PartialDiagnostic Warning =
         S.PDiag(diag::warn_locks_not_released)
           << MissingLock.getName() << FunName;

clang/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;
+  }
+};
+
+

clang/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<void (UnFoo::*methptr)()>
+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());
+  }
+};
+
+