On Mon, Mar 5, 2018 at 11:05 PM George Karpenkov via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> Author: george.karpenkov > Date: Mon Mar 5 14:03:32 2018 > New Revision: 326746 > > URL: http://llvm.org/viewvc/llvm-project?rev=326746&view=rev > Log: > [analyzer] AST-matching checker to detect global central dispatch > performance anti-pattern > > rdar://37312818 > > NB: The checker does not care about the ordering of callbacks, see the > relevant FIXME in tests. > > Differential Revision: https://reviews.llvm.org/D44059 > > Added: > cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp > cfe/trunk/test/gcdasyncsemaphorechecker_test.m > The test doesn't belong here. The right place seems to be test/Analysis/. > Modified: > cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td > cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt > > Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=326746&r1=326745&r2=326746&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original) > +++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Mon Mar 5 > 14:03:32 2018 > @@ -610,6 +610,13 @@ def ObjCSuperDeallocChecker : Checker<"S > > } // end "osx.cocoa" > > +let ParentPackage = OSXAlpha in { > + > +def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">, > + HelpText<"Checker for performance anti-pattern when using semaphors > from async API">, > + DescFile<"GCDAsyncSemaphoreChecker.cpp">; > +} // end "alpha.osx" > + > let ParentPackage = CocoaAlpha in { > > def InstanceVariableInvalidation : > Checker<"InstanceVariableInvalidation">, > > Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=326746&r1=326745&r2=326746&view=diff > > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Mon Mar 5 > 14:03:32 2018 > @@ -37,6 +37,7 @@ add_clang_library(clangStaticAnalyzerChe > DynamicTypeChecker.cpp > ExprInspectionChecker.cpp > FixedAddressChecker.cpp > + GCDAsyncSemaphoreChecker.cpp > GenericTaintChecker.cpp > GTestChecker.cpp > IdenticalExprChecker.cpp > > Added: cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp?rev=326746&view=auto > > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp > (added) > +++ cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp Mon > Mar 5 14:03:32 2018 > @@ -0,0 +1,154 @@ > +//===- GCDAsyncSemaphoreChecker.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 > +// antipattern when synchronous API is emulated from asynchronous > callbacks > +// using a semaphor: > +// > +// dispatch_semapshore_t sema = dispatch_semaphore_create(0); > + > +// AnyCFunctionCall(^{ > +// // code… > +// dispatch_semapshore_signal(sema); > +// }) > +// dispatch_semapshore_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. > +// 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> { > +public: > + void checkASTCodeBody(const Decl *D, > + AnalysisManager &AM, > + BugReporter &BR) const; > +}; > + > +class Callback : public MatchFinder::MatchCallback { > + BugReporter &BR; > + const GCDAsyncSemaphoreChecker *C; > + AnalysisDeclContext *ADC; > + > +public: > + Callback(BugReporter &BR, > + AnalysisDeclContext *ADC, > + const GCDAsyncSemaphoreChecker *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, > + 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)) > + if (ND->getName().startswith("test")) > + 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), > + hasRHS(SemaphoreCreateM)))); > + > + auto SemaphoreWaitM = forEachDescendant( > + callExpr( > + allOf( > + callsName("dispatch_semaphore_wait"), > + equalsBoundArgDecl(0, SemaphoreBinding) > + ) > + ).bind(WarningBinding)); > + > + auto AcceptsBlockM = > + forEachDescendant(callExpr(hasAnyArgument(hasType( > + hasCanonicalType(blockPointerType()) > + )))); > + > + auto BlockSignallingM = > + forEachDescendant(callExpr(hasAnyArgument(hasDescendant(callExpr( > + allOf( > + callsName("dispatch_semaphore_signal"), > + equalsBoundArgDecl(0, SemaphoreBinding) > + )))))); > + > + auto FinalM = compoundStmt( > + SemaphoreBindingM, > + SemaphoreWaitM, > + AcceptsBlockM, > + BlockSignallingM); > + > + MatchFinder F; > + Callback CB(BR, AM.getAnalysisDeclContext(D), this); > + > + F.addMatcher(FinalM, &CB); > + F.match(*D->getBody(), AM.getASTContext()); > +} > + > +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", > + PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC), > + SW->getSourceRange()); > +} > + > +} > + > +void ento::registerGCDAsyncSemaphoreChecker(CheckerManager &Mgr) { > + Mgr.registerChecker<GCDAsyncSemaphoreChecker>(); > +} > > Added: cfe/trunk/test/gcdasyncsemaphorechecker_test.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/gcdasyncsemaphorechecker_test.m?rev=326746&view=auto > > ============================================================================== > --- cfe/trunk/test/gcdasyncsemaphorechecker_test.m (added) > +++ cfe/trunk/test/gcdasyncsemaphorechecker_test.m Mon Mar 5 14:03:32 2018 > @@ -0,0 +1,169 @@ > +// RUN: %clang_analyze_cc1 > -analyzer-checker=core,alpha.osx.GCDAsyncSemaphore %s -fblocks -verify > +// > +typedef int dispatch_semaphore_t; > +typedef void (^block_t)(); > + > +dispatch_semaphore_t dispatch_semaphore_create(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 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}} > +} > + > +// 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_t sema2 = dispatch_semaphore_create(0); > + > + func(^{ > + dispatch_semaphore_signal(sema2); > + }); > + dispatch_semaphore_wait(sema2, 100); // expected-warning{{Possible > semaphore performance anti-pattern}} > +} > + > +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}} > +} > + > +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}} > + 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}} > +} > + > +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}} > +} > + > +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}} > +} > + > +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}} > +} > + > +void differentsemaphoreok() { > + dispatch_semaphore_t sema1 = dispatch_semaphore_create(0); > + dispatch_semaphore_t sema2 = dispatch_semaphore_create(0); > + > + func(^{ > + dispatch_semaphore_signal(sema1); > + }); > + dispatch_semaphore_wait(sema2, 100); // no-warning > +} > + > +void nosignalok() { > + dispatch_semaphore_t sema1 = dispatch_semaphore_create(0); > + dispatch_semaphore_wait(sema1, 100); > +} > + > +void nowaitok() { > + dispatch_semaphore_t sema = dispatch_semaphore_create(0); > + func(^{ > + dispatch_semaphore_signal(sema); > + }); > +} > + > +void noblockok() { > + dispatch_semaphore_t sema = dispatch_semaphore_create(0); > + dispatch_semaphore_signal(sema); > + dispatch_semaphore_wait(sema, 100); > +} > + > +void storedblockok() { > + dispatch_semaphore_t sema = dispatch_semaphore_create(0); > + block_t b = ^{ > + dispatch_semaphore_signal(sema); > + }; > + dispatch_semaphore_wait(sema, 100); > +} > + > +void passed_semaphore_ok(dispatch_semaphore_t sema) { > + func(^{ > + dispatch_semaphore_signal(sema); > + }); > + dispatch_semaphore_wait(sema, 100); > +} > + > +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}} > +} > + > + > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
