From 89132b0bb6cab30e9ac3a711210373f5eec18568 Mon Sep 17 00:00:00 2001 From: George Karpenkov Date: Mon, 12 Mar 2018 18:27:36 +0000 Subject: [PATCH] [analyzer] Move the GCDAsyncSemaphoreChecker to optin.performance rdar://38383753 Differential Revision: https://reviews.llvm.org/D44228 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@327309 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/StaticAnalyzer/Checkers/Checkers.td | 10 +-- lib/StaticAnalyzer/Checkers/CMakeLists.txt | 2 +- ...eChecker.cpp => GCDAntipatternChecker.cpp} | 43 +++++++----- ...er_test.m => gcdantipatternchecker_test.m} | 70 ++++++++++++++----- 4 files changed, 82 insertions(+), 43 deletions(-) rename lib/StaticAnalyzer/Checkers/{GCDAsyncSemaphoreChecker.cpp => GCDAntipatternChecker.cpp} (78%) rename test/Analysis/{gcdasyncsemaphorechecker_test.m => gcdantipatternchecker_test.m} (58%) diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td index 3b94a9b161..0fb1b08e1b 100644 --- a/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -610,12 +610,12 @@ def ObjCSuperDeallocChecker : Checker<"SuperDealloc">, } // 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 { diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 9b42a5581e..baf36e03d6 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -37,7 +37,7 @@ add_clang_library(clangStaticAnalyzerCheckers DynamicTypeChecker.cpp ExprInspectionChecker.cpp FixedAddressChecker.cpp - GCDAsyncSemaphoreChecker.cpp + GCDAntipatternChecker.cpp GenericTaintChecker.cpp GTestChecker.cpp IdenticalExprChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp b/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp similarity index 78% rename from lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp rename to lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp index eda7a5fcd1..51a3fc1938 100644 --- a/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp @@ -1,4 +1,4 @@ -//===- GCDAsyncSemaphoreChecker.cpp -----------------------------*- C++ -*-==// +//===- GCDAntipatternChecker.cpp ---------------------------------*- C++ -*-==// // // The LLVM Compiler Infrastructure // @@ -7,20 +7,20 @@ // //===----------------------------------------------------------------------===// // -// 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. // @@ -37,8 +37,6 @@ #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; @@ -47,7 +45,7 @@ namespace { const char *WarningBinding = "semaphore_wait"; -class GCDAsyncSemaphoreChecker : public Checker { +class GCDAntipatternChecker : public Checker { public: void checkASTCodeBody(const Decl *D, AnalysisManager &AM, @@ -56,13 +54,13 @@ public: 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; }; @@ -83,7 +81,7 @@ auto bindAssignmentToDecl(const char *DeclName) -> decltype(hasLHS(expr())) { declRefExpr(to(varDecl().bind(DeclName))))); } -void GCDAsyncSemaphoreChecker::checkASTCodeBody(const Decl *D, +void GCDAntipatternChecker::checkASTCodeBody(const Decl *D, AnalysisManager &AM, BugReporter &BR) const { @@ -93,6 +91,14 @@ void GCDAsyncSemaphoreChecker::checkASTCodeBody(const Decl *D, if (StringRef(DeclName).startswith("test")) return; } + if (const auto *OD = dyn_cast(D)) { + if (const auto *CD = dyn_cast(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")); @@ -146,14 +152,15 @@ void Callback::run(const MatchFinder::MatchResult &Result) { 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; consider " + "using a synchronous API or changing the caller to be asynchronous", PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC), SW->getSourceRange()); } } -void ento::registerGCDAsyncSemaphoreChecker(CheckerManager &Mgr) { - Mgr.registerChecker(); +void ento::registerGCDAntipattern(CheckerManager &Mgr) { + Mgr.registerChecker(); } diff --git a/test/Analysis/gcdasyncsemaphorechecker_test.m b/test/Analysis/gcdantipatternchecker_test.m similarity index 58% rename from test/Analysis/gcdasyncsemaphorechecker_test.m rename to test/Analysis/gcdantipatternchecker_test.m index ff434d3652..19f446787c 100644 --- a/test/Analysis/gcdasyncsemaphorechecker_test.m +++ b/test/Analysis/gcdantipatternchecker_test.m @@ -1,4 +1,4 @@ -// 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 {} @@ -27,7 +27,7 @@ void use_semaphor_antipattern() { 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. @@ -47,14 +47,14 @@ void use_semaphor_antipattern_multiple_times() { 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() { @@ -64,8 +64,8 @@ void use_semaphor_antipattern_multiple_wait() { 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() { @@ -73,7 +73,7 @@ void warn_incorrect_order() { // 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); }); @@ -85,7 +85,7 @@ void warn_w_typedef() { 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() { @@ -100,7 +100,7 @@ void warn_nested_ast() { 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() { @@ -110,7 +110,7 @@ void use_semaphore_assignment() { 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() { @@ -120,7 +120,7 @@ void use_semaphore_assignment_init() { 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() { @@ -172,17 +172,17 @@ void warn_with_cast() { 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); @@ -190,7 +190,7 @@ void warn_with_cast() { 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 { @@ -212,23 +212,55 @@ void warn_with_cast() { [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 -- 2.40.0