[analyzer] Move the GCDAsyncSemaphoreChecker to optin.performance

[dcoughlin]

I put some post-commit comments in https://reviews.llvm.org/D44059 about diagnostic text and naming that you should address as part of graduating this out of alpha.

[george.karpenkov]

@dcoughlin Integrated post-commit feedback.

[dcoughlin]

LGTM, but I do think Artem is right that we should push the user in the right direction here. There is a suggested addendum inline.

lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp

156

I agree with Artem that we should push the user in the right direction here. Let's add "; consider using a synchronous API or changing the caller to be asynchronous" to the description.

include/clang/StaticAnalyzer/Checkers/Checkers.td

def ObjCDeallocChecker : Checker<"Dealloc">,
   DescFile<"CheckObjCDealloc.cpp">;
 
 def ObjCSuperDeallocChecker : Checker<"SuperDealloc">,
   HelpText<"Warn about improper use of '[super dealloc]' in Objective-C">,
   DescFile<"ObjCSuperDeallocChecker.cpp">;
 
 } // end "osx.cocoa"
 
-let ParentPackage = OSXAlpha in {
+let ParentPackage = Performance in {
 
-def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,
-  HelpText<"Checker for performance anti-pattern when using semaphors from async API">,
-  DescFile<"GCDAsyncSemaphoreChecker.cpp">;
-} // end "alpha.osx"
+def GCDAntipattern : Checker<"GCDAntipattern">,
+  HelpText<"Check for performance anti-patterns when using Grand Central Dispatch">,
+  DescFile<"GCDAntipatternChecker.cpp">;
+} // end "optin.performance"
 
 let ParentPackage = CocoaAlpha in {
 
 def InstanceVariableInvalidation : Checker<"InstanceVariableInvalidation">,
   HelpText<"Check that the invalidatable instance variables are invalidated in the methods annotated with objc_instance_variable_invalidator">,
   DescFile<"IvarInvalidationChecker.cpp">;
 
 def MissingInvalidationMethod : Checker<"MissingInvalidationMethod">,

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
-  GCDAsyncSemaphoreChecker.cpp
+  GCDAntipatternChecker.cpp
   GenericTaintChecker.cpp
   GTestChecker.cpp
   IdenticalExprChecker.cpp
   IteratorChecker.cpp
   IvarInvalidationChecker.cpp
   LLVMConventionsChecker.cpp
   LocalizationChecker.cpp
   MacOSKeychainAPIChecker.cpp

lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp

-//===- GCDAsyncSemaphoreChecker.cpp -----------------------------*- C++ -*-==//
+//===- GCDAntipatternChecker.cpp ---------------------------------*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
-// This file defines GCDAsyncSemaphoreChecker which checks against a common
+// This file defines GCDAntipatternChecker which checks against a common
 // antipattern when synchronous API is emulated from asynchronous callbacks
-// using a semaphor:
+// using a semaphore:
+//
+//   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 //
-//   dispatch_semapshore_t sema = dispatch_semaphore_create(0);
-
 //   AnyCFunctionCall(^{
 //     // code…
-//     dispatch_semapshore_signal(sema);
+//     dispatch_semaphore_signal(sema);
 //   })
-//   dispatch_semapshore_wait(sema, *)
+//   dispatch_semaphore_wait(sema, *)
 //
 // Such code is a common performance problem, due to inability of GCD to
-// properly handle QoS when a combination of queues and semaphors is used.
+// properly handle QoS when a combination of queues and semaphores is used.
 // Good code would either use asynchronous API (when available), or perform
 // the necessary action in asynchronous callback.
 //
 // Currently, the check is performed using a simple heuristical AST pattern
 // matching.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangSACheckers.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "llvm/Support/Debug.h"
 
-#define DEBUG_TYPE "gcdasyncsemaphorechecker"
-
 using namespace clang;
 using namespace ento;
 using namespace ast_matchers;
 
 namespace {
 
 const char *WarningBinding = "semaphore_wait";
 
-class GCDAsyncSemaphoreChecker : public Checker<check::ASTCodeBody> {
+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 GCDAsyncSemaphoreChecker *C;
+  const GCDAntipatternChecker *C;
   AnalysisDeclContext *ADC;
 
 public:
   Callback(BugReporter &BR,
            AnalysisDeclContext *ADC,
-           const GCDAsyncSemaphoreChecker *C) : BR(BR), C(C), ADC(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 GCDAsyncSemaphoreChecker::checkASTCodeBody(const Decl *D,
+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.
   if (const auto* ND = dyn_cast<NamedDecl>(D)) {
     std::string DeclName = ND->getNameAsString();
     if (StringRef(DeclName).startswith("test"))
       return;
   }
+  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;
+    }
+  }
 
   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),

 
 void Callback::run(const MatchFinder::MatchResult &Result) {
   const auto *SW = Result.Nodes.getNodeAs<CallExpr>(WarningBinding);
   assert(SW);
   BR.EmitBasicReport(
       ADC->getDecl(), C,
       /*Name=*/"Semaphore performance anti-pattern",
       /*Category=*/"Performance",
-      "Possible semaphore performance anti-pattern: wait on a semaphore "
-      "signalled to in a callback",
+      "Waiting on a semaphore with Grand Central Dispatch creates useless "
+      "threads and is subject to priority inversion",
       PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC),
       SW->getSourceRange());
 }
 
 }
 
-void ento::registerGCDAsyncSemaphoreChecker(CheckerManager &Mgr) {
-  Mgr.registerChecker<GCDAsyncSemaphoreChecker>();
+void ento::registerGCDAntipattern(CheckerManager &Mgr) {
+  Mgr.registerChecker<GCDAntipatternChecker>();
 }

test/Analysis/gcdantipatternchecker_test.m

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.osx.GCDAsyncSemaphore %s -fblocks -verify
+// 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;

 int coin();
 
 void use_semaphor_antipattern() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 }
 
 // It's OK to use pattern in tests.
 // We simply match the containing function name against ^test.
 void test_no_warning() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
   dispatch_semaphore_wait(sema, 100);
 }
 
 void use_semaphor_antipattern_multiple_times() {
   dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema1);
   });
-  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 
   dispatch_semaphore_t sema2 = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema2);
   });
-  dispatch_semaphore_wait(sema2, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema2, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 }
 
 void use_semaphor_antipattern_multiple_wait() {
   dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema1);
   });
   // FIXME: multiple waits on same semaphor should not raise a warning.
-  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
-  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
+  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 }
 
 void warn_incorrect_order() {
   // FIXME: ASTMatchers do not allow ordered matching, so would match even
   // if out of order.
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
   func(^{
       dispatch_semaphore_signal(sema);
   });
 }
 
 void warn_w_typedef() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func_w_typedef(^{
       dispatch_semaphore_signal(sema);
   });
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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 warn_nested_ast() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   if (coin()) {
     func(^{
          dispatch_semaphore_signal(sema);
          });
   } else {
     func(^{
          dispatch_semaphore_signal(sema);
          });
   }
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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_semaphore_assignment() {
   dispatch_semaphore_t sema;
   sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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_semaphore_assignment_init() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
   sema = dispatch_semaphore_create(1);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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 differentsemaphoreok() {
   dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);
   dispatch_semaphore_t sema2 = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema1);

 }
 
 void warn_with_cast() {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal((int)sema);
   });
-  dispatch_semaphore_wait((int)sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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 Test1 : NSObject
+@interface MyInterface1 : NSObject
 -(void)use_method_warn;
 -(void)use_objc_callback_warn;
 -(void)testNoWarn;
 -(void)acceptBlock:(block_t)callback;
 @end
 
-@implementation Test1
+@implementation MyInterface1
 
 -(void)use_method_warn {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
-  dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  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)testNoWarn {
   dispatch_semaphore_t sema = dispatch_semaphore_create(0);
 
   func(^{
       dispatch_semaphore_signal(sema);
   });
   dispatch_semaphore_wait(sema, 100);
 }
 
 -(void)acceptBlock:(block_t) callback {
   callback();
 }
 
 -(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{{Possible semaphore performance anti-pattern}}
+  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_objc_and_c_callback(Test1 *t) {
+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{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
 
   dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);
 
   [t acceptBlock:^{
       dispatch_semaphore_signal(sema1);
   }];
-  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}
+  dispatch_semaphore_wait(sema1, 100); // expected-warning{{Waiting on a semaphore with Grand Central Dispatch creates useless threads and is subject to priority inversion}}
+}
+@end
+
+// No warnings: class name contains "test"
+@interface Test1 : NSObject
+-(void)use_method_warn;
+@end
+
+@implementation Test1
+-(void)use_method_warn {
+  dispatch_semaphore_t sema = dispatch_semaphore_create(0);
+
+  func(^{
+      dispatch_semaphore_signal(sema);
+  });
+  dispatch_semaphore_wait(sema, 100);
 }
+@end
+
 
+// No warnings: class name contains "mock"
+@interface Mock1 : NSObject
+-(void)use_method_warn;
+@end
+
+@implementation Mock1
+-(void)use_method_warn {
+  dispatch_semaphore_t sema = dispatch_semaphore_create(0);
+
+  func(^{
+      dispatch_semaphore_signal(sema);
+  });
+  dispatch_semaphore_wait(sema, 100);
+}
 @end