This revision was automatically updated to reflect the committed changes.
Closed by commit rC326746: [analyzer] AST-matching checker to detect global
central dispatch performance… (authored by george.karpenkov, committed by ).
Herald added a subscriber: cfe-commits.
Repository:
rC Clang
https://reviews.llvm.org/D44059
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
test/gcdasyncsemaphorechecker_test.m
Index: test/gcdasyncsemaphorechecker_test.m
===================================================================
--- test/gcdasyncsemaphorechecker_test.m
+++ test/gcdasyncsemaphorechecker_test.m
@@ -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}}
+}
+
+
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===================================================================
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -37,6 +37,7 @@
DynamicTypeChecker.cpp
ExprInspectionChecker.cpp
FixedAddressChecker.cpp
+ GCDAsyncSemaphoreChecker.cpp
GenericTaintChecker.cpp
GTestChecker.cpp
IdenticalExprChecker.cpp
Index: lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp
@@ -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>();
+}
Index: include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -610,6 +610,13 @@
} // 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">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
