Index: cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp @@ -43,7 +43,8 @@ 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 { public: @@ -52,19 +53,6 @@ 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))); @@ -81,24 +69,28 @@ 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(D)) { std::string DeclName = ND->getNameAsString(); if (StringRef(DeclName).startswith("test")) - return; + return true; } 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; + return true; } } + return false; +} + +static auto findGCDAntiPatternWithSemaphore() -> decltype(compoundStmt()) { const char *SemaphoreBinding = "semaphore_name"; auto SemaphoreCreateM = callExpr(callsName("dispatch_semaphore_create")); @@ -109,13 +101,51 @@ 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()) @@ -123,40 +153,71 @@ 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(WarningBinding); +static void emitDiagnostics(const BoundNodes &Nodes, + const char* Type, + BugReporter &BR, + AnalysisDeclContext *ADC, + const GCDAntipatternChecker *Checker) { + const auto *SW = Nodes.getNodeAs(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", + /*Category=*/"Performance", + OS.str(), + PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC), + SW->getSourceRange()); +} + +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); } } Index: cfe/trunk/test/Analysis/gcdantipatternchecker_test.m =================================================================== --- cfe/trunk/test/Analysis/gcdantipatternchecker_test.m +++ cfe/trunk/test/Analysis/gcdantipatternchecker_test.m @@ -10,9 +10,16 @@ @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); @@ -179,6 +186,7 @@ -(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; @@ -230,6 +238,16 @@ 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); @@ -279,3 +297,39 @@ 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}} +}