[analyzer] Extend GCDAntipatternChecker to match group_enter/group_leave pattern

[george.karpenkov]

Summary

rdar://38480416

[dcoughlin]

Looks good to me!

cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp

 #include "llvm/Support/Debug.h"
 
 using namespace clang;
 using namespace ento;
 using namespace ast_matchers;
 
 namespace {
 
-const char *WarningBinding = "semaphore_wait";
+// ID of a node at which the diagnostic would be emitted.
+const char *WarnAtNode = "waitcall";
 
 class GCDAntipatternChecker : public Checker<check::ASTCodeBody> {
 public:
   void checkASTCodeBody(const Decl *D,
                         AnalysisManager &AM,
                         BugReporter &BR) const;
 };
 
-class Callback : public MatchFinder::MatchCallback {
-  BugReporter &BR;
-  const GCDAntipatternChecker *C;
-  AnalysisDeclContext *ADC;
-
-public:
-  Callback(BugReporter &BR,
-           AnalysisDeclContext *ADC,
-           const GCDAntipatternChecker *C) : BR(BR), C(C), ADC(ADC) {}
-
-  virtual void run(const MatchFinder::MatchResult &Result) override;
-};
-
 auto callsName(const char *FunctionName)
     -> decltype(callee(functionDecl())) {
   return callee(functionDecl(hasName(FunctionName)));
 }
 
 auto equalsBoundArgDecl(int ArgIdx, const char *DeclName)
     -> decltype(hasArgument(0, expr())) {
   return hasArgument(ArgIdx, ignoringParenCasts(declRefExpr(
                                  to(varDecl(equalsBoundNode(DeclName))))));
 }
 
 auto bindAssignmentToDecl(const char *DeclName) -> decltype(hasLHS(expr())) {
   return hasLHS(ignoringParenImpCasts(
                          declRefExpr(to(varDecl().bind(DeclName)))));
 }
 
-void GCDAntipatternChecker::checkASTCodeBody(const Decl *D,
-                                               AnalysisManager &AM,
-                                               BugReporter &BR) const {
-
-  // The pattern is very common in tests, and it is OK to use it there.
+/// The pattern is very common in tests, and it is OK to use it there.
+/// We have to heuristics for detecting tests: method name starts with "test"
+/// (used in XCTest), and a class name contains "mock" or "test" (used in
+/// helpers which are not tests themselves, but used exclusively in tests).
+static bool isTest(const Decl *D) {
   if (const auto* ND = dyn_cast<NamedDecl>(D)) {
     std::string DeclName = ND->getNameAsString();
     if (StringRef(DeclName).startswith("test"))
-      return;
+      return true;
   }
   if (const auto *OD = dyn_cast<ObjCMethodDecl>(D)) {
     if (const auto *CD = dyn_cast<ObjCContainerDecl>(OD->getParent())) {
       std::string ContainerName = CD->getNameAsString();
       StringRef CN(ContainerName);
       if (CN.contains_lower("test") || CN.contains_lower("mock"))
-        return;
+        return true;
     }
   }
+  return false;
+}
+
+static auto findGCDAntiPatternWithSemaphore() -> decltype(compoundStmt()) {
 
   const char *SemaphoreBinding = "semaphore_name";
   auto SemaphoreCreateM = callExpr(callsName("dispatch_semaphore_create"));
 
   auto SemaphoreBindingM = anyOf(
       forEachDescendant(
           varDecl(hasDescendant(SemaphoreCreateM)).bind(SemaphoreBinding)),
       forEachDescendant(binaryOperator(bindAssignmentToDecl(SemaphoreBinding),
                      hasRHS(SemaphoreCreateM))));
 
+  auto HasBlockArgumentM = hasAnyArgument(hasType(
+            hasCanonicalType(blockPointerType())
+            ));
+
+  auto ArgCallsSignalM = hasAnyArgument(stmt(hasDescendant(callExpr(
+          allOf(
+              callsName("dispatch_semaphore_signal"),
+              equalsBoundArgDecl(0, SemaphoreBinding)
+              )))));
+
+  auto HasBlockAndCallsSignalM = allOf(HasBlockArgumentM, ArgCallsSignalM);
+
+  auto HasBlockCallingSignalM =
+    forEachDescendant(
+      stmt(anyOf(
+        callExpr(HasBlockAndCallsSignalM),
+        objcMessageExpr(HasBlockAndCallsSignalM)
+           )));
+
   auto SemaphoreWaitM = forEachDescendant(
     callExpr(
       allOf(
         callsName("dispatch_semaphore_wait"),
         equalsBoundArgDecl(0, SemaphoreBinding)
       )
-    ).bind(WarningBinding));
+    ).bind(WarnAtNode));
+
+  return compoundStmt(
+      SemaphoreBindingM, HasBlockCallingSignalM, SemaphoreWaitM);
+}
+
+static auto findGCDAntiPatternWithGroup() -> decltype(compoundStmt()) {
+
+  const char *GroupBinding = "group_name";
+  auto DispatchGroupCreateM = callExpr(callsName("dispatch_group_create"));
+
+  auto GroupBindingM = anyOf(
+      forEachDescendant(
+          varDecl(hasDescendant(DispatchGroupCreateM)).bind(GroupBinding)),
+      forEachDescendant(binaryOperator(bindAssignmentToDecl(GroupBinding),
+                     hasRHS(DispatchGroupCreateM))));
+
+  auto GroupEnterM = forEachDescendant(
+      stmt(callExpr(allOf(callsName("dispatch_group_enter"),
+                          equalsBoundArgDecl(0, GroupBinding)))));
 
   auto HasBlockArgumentM = hasAnyArgument(hasType(
             hasCanonicalType(blockPointerType())
             ));
 
   auto ArgCallsSignalM = hasAnyArgument(stmt(hasDescendant(callExpr(
           allOf(
-              callsName("dispatch_semaphore_signal"),
-              equalsBoundArgDecl(0, SemaphoreBinding)
+              callsName("dispatch_group_leave"),
+              equalsBoundArgDecl(0, GroupBinding)
               )))));
 
-  auto HasBlockAndCallsSignalM = allOf(HasBlockArgumentM, ArgCallsSignalM);
+  auto HasBlockAndCallsLeaveM = allOf(HasBlockArgumentM, ArgCallsSignalM);
 
   auto AcceptsBlockM =
     forEachDescendant(
       stmt(anyOf(
-        callExpr(HasBlockAndCallsSignalM),
-        objcMessageExpr(HasBlockAndCallsSignalM)
+        callExpr(HasBlockAndCallsLeaveM),
+        objcMessageExpr(HasBlockAndCallsLeaveM)
            )));
 
-  auto FinalM = compoundStmt(SemaphoreBindingM, SemaphoreWaitM, AcceptsBlockM);
-
-  MatchFinder F;
-  Callback CB(BR, AM.getAnalysisDeclContext(D), this);
+  auto GroupWaitM = forEachDescendant(
+    callExpr(
+      allOf(
+        callsName("dispatch_group_wait"),
+        equalsBoundArgDecl(0, GroupBinding)
+      )
+    ).bind(WarnAtNode));
 
-  F.addMatcher(FinalM, &CB);
-  F.match(*D->getBody(), AM.getASTContext());
+  return compoundStmt(GroupBindingM, GroupEnterM, AcceptsBlockM, GroupWaitM);
 }
 
-void Callback::run(const MatchFinder::MatchResult &Result) {
-  const auto *SW = Result.Nodes.getNodeAs<CallExpr>(WarningBinding);
+static void emitDiagnostics(const BoundNodes &Nodes,
+                            const char* Type,
+                            BugReporter &BR,
+                            AnalysisDeclContext *ADC,
+                            const GCDAntipatternChecker *Checker) {
+  const auto *SW = Nodes.getNodeAs<CallExpr>(WarnAtNode);
   assert(SW);
+
+  std::string Diagnostics;
+  llvm::raw_string_ostream OS(Diagnostics);
+  OS << "Waiting on a " << Type << " with Grand Central Dispatch creates "
+     << "useless threads and is subject to priority inversion; consider "
+     << "using a synchronous API or changing the caller to be asynchronous";
+
   BR.EmitBasicReport(
-      ADC->getDecl(), C,
-      /*Name=*/"Semaphore performance anti-pattern",
-      /*Category=*/"Performance",
-      "Waiting on a semaphore with Grand Central Dispatch creates useless "
-      "threads and is subject to priority inversion; consider "
-      "using a synchronous API or changing the caller to be asynchronous",
-      PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC),
-      SW->getSourceRange());
+    ADC->getDecl(),
+    Checker,
+    /*Name=*/"GCD performance anti-pattern",
+    OS.str(),
 }
 
+void GCDAntipatternChecker::checkASTCodeBody(const Decl *D,
+                                             AnalysisManager &AM,
+                                             BugReporter &BR) const {
+  if (isTest(D))
+    return;
+
+  AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D);
+
+  auto SemaphoreMatcherM = findGCDAntiPatternWithSemaphore();
+  auto Matches = match(SemaphoreMatcherM, *D->getBody(), AM.getASTContext());
+  for (BoundNodes Match : Matches)
+    emitDiagnostics(Match, "semaphore", BR, ADC, this);
+
+  auto GroupMatcherM = findGCDAntiPatternWithGroup();
+  Matches = match(GroupMatcherM, *D->getBody(), AM.getASTContext());
+  for (BoundNodes Match : Matches)
+    emitDiagnostics(Match, "group", BR, ADC, this);
+}
+
 }
 
 void ento::registerGCDAntipattern(CheckerManager &Mgr) {
   Mgr.registerChecker<GCDAntipatternChecker>();
 }

cfe/trunk/test/Analysis/gcdantipatternchecker_test.m

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,optin.performance.GCDAntipattern %s -fblocks -verify
 typedef signed char BOOL;
 @protocol NSObject  - (BOOL)isEqual:(id)object; @end
 @interface NSObject <NSObject> {}
 +(id)alloc;
 -(id)init;
 -(id)autorelease;
 -(id)copy;
 -(id)retain;
 @end
 
 typedef int dispatch_semaphore_t;
+typedef int dispatch_group_t;
 typedef void (^block_t)();
 
 dispatch_semaphore_t dispatch_semaphore_create(int);
+dispatch_group_t dispatch_group_create();
+void dispatch_group_enter(dispatch_group_t);
+void dispatch_group_leave(dispatch_group_t);
+void dispatch_group_wait(dispatch_group_t, int);
+
+
 void dispatch_semaphore_wait(dispatch_semaphore_t, int);
 void dispatch_semaphore_signal(dispatch_semaphore_t);
 
 void func(void (^)(void));
 void func_w_typedef(block_t);
 
 int coin();
 
void warn_with_cast() {
   });
   dispatch_semaphore_wait((int)sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 }
 
 @interface MyInterface1 : NSObject
 -(void)use_method_warn;
 -(void) pass_block_as_second_param_warn;
 -(void)use_objc_callback_warn;
+-(void) use_dispatch_group;
 -(void)testNoWarn;
 -(void)acceptBlock:(block_t)callback;
 -(void)flag:(int)flag acceptBlock:(block_t)callback;
 @end
 
 @implementation MyInterface1
 
 -(void)use_method_warn {
-(void)use_objc_callback_warn {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   [self acceptBlock:^{
       dispatch_semaphore_signal(sema);
   }];
   dispatch_semaphore_wait(sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 }
 
+-(void)use_dispatch_group {
+  dispatch_group_t group = dispatch_group_create();
+  dispatch_group_enter(group);
+  [self acceptBlock:^{
+    dispatch_group_leave(group);
+  }];
+  dispatch_group_wait(group, 100); // expected-warning{{Waiting on a group with Grand Central Dispatch}}
+
+}
+
 void use_objc_and_c_callback(MyInterface1 *t) {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
   dispatch_semaphore_wait(sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 
-(void)use_method_warn {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
   dispatch_semaphore_wait(sema, 100);
 }
 @end
+
+void dispatch_group_wait_func(MyInterface1 *M) {
+  dispatch_group_t group = dispatch_group_create();
+  dispatch_group_enter(group);
+
+  func(^{
+      dispatch_group_leave(group);
+  });
+  dispatch_group_wait(group, 100); // expected-warning{{Waiting on a group with Grand Central Dispatch}}
+}
+
+
+void dispatch_group_wait_cfunc(MyInterface1 *M) {
+  dispatch_group_t group = dispatch_group_create();
+  dispatch_group_enter(group);
+  [M acceptBlock:^{
+    dispatch_group_leave(group);
+  }];
+  dispatch_group_wait(group, 100); // expected-warning{{Waiting on a group with Grand Central Dispatch}}
+}
+
+void dispatch_group_and_semaphore_use(MyInterface1 *M) {
+  dispatch_group_t group = dispatch_group_create();
+  dispatch_group_enter(group);
+  [M acceptBlock:^{
+    dispatch_group_leave(group);
+  }];
+  dispatch_group_wait(group, 100); // expected-warning{{Waiting on a group with Grand Central Dispatch}}
+
+  dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);
+
+  [M acceptBlock:^{
+      dispatch_semaphore_signal(sema1);
+  }];
+  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
+}