r326746 - [analyzer] AST-matching checker to detect global central dispatch performance anti-pattern
Alexander Kornienko via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 6 02:03:06 PST 2018
On Mon, Mar 5, 2018 at 11:05 PM George Karpenkov via cfe-commits <
cfe-commits at 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 at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180306/38633502/attachment-0001.html>
More information about the cfe-commits
mailing list