<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Mar 5, 2018 at 11:05 PM George Karpenkov via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: george.karpenkov<br>
Date: Mon Mar 5 14:03:32 2018<br>
New Revision: 326746<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=326746&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=326746&view=rev</a><br>
Log:<br>
[analyzer] AST-matching checker to detect global central dispatch performance anti-pattern<br>
<br>
rdar://37312818<br>
<br>
NB: The checker does not care about the ordering of callbacks, see the<br>
relevant FIXME in tests.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D44059" rel="noreferrer" target="_blank">https://reviews.llvm.org/D44059</a><br>
<br>
Added:<br>
cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp<br>
cfe/trunk/test/gcdasyncsemaphorechecker_test.m<br></blockquote><div><br></div><div>The test doesn't belong here. The right place seems to be test/Analysis/.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Modified:<br>
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td<br>
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt<br>
<br>
Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=326746&r1=326745&r2=326746&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=326746&r1=326745&r2=326746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)<br>
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Mon Mar 5 14:03:32 2018<br>
@@ -610,6 +610,13 @@ def ObjCSuperDeallocChecker : Checker<"S<br>
<br>
} // end "osx.cocoa"<br>
<br>
+let ParentPackage = OSXAlpha in {<br>
+<br>
+def GCDAsyncSemaphoreChecker : Checker<"GCDAsyncSemaphore">,<br>
+ HelpText<"Checker for performance anti-pattern when using semaphors from async API">,<br>
+ DescFile<"GCDAsyncSemaphoreChecker.cpp">;<br>
+} // end "alpha.osx"<br>
+<br>
let ParentPackage = CocoaAlpha in {<br>
<br>
def InstanceVariableInvalidation : Checker<"InstanceVariableInvalidation">,<br>
<br>
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=326746&r1=326745&r2=326746&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=326746&r1=326745&r2=326746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original)<br>
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Mon Mar 5 14:03:32 2018<br>
@@ -37,6 +37,7 @@ add_clang_library(clangStaticAnalyzerChe<br>
DynamicTypeChecker.cpp<br>
ExprInspectionChecker.cpp<br>
FixedAddressChecker.cpp<br>
+ GCDAsyncSemaphoreChecker.cpp<br>
GenericTaintChecker.cpp<br>
GTestChecker.cpp<br>
IdenticalExprChecker.cpp<br>
<br>
Added: cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp?rev=326746&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp?rev=326746&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp (added)<br>
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/GCDAsyncSemaphoreChecker.cpp Mon Mar 5 14:03:32 2018<br>
@@ -0,0 +1,154 @@<br>
+//===- GCDAsyncSemaphoreChecker.cpp -----------------------------*- C++ -*-==//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+//<br>
+// This file defines GCDAsyncSemaphoreChecker which checks against a common<br>
+// antipattern when synchronous API is emulated from asynchronous callbacks<br>
+// using a semaphor:<br>
+//<br>
+// dispatch_semapshore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+// AnyCFunctionCall(^{<br>
+// // code…<br>
+// dispatch_semapshore_signal(sema);<br>
+// })<br>
+// dispatch_semapshore_wait(sema, *)<br>
+//<br>
+// Such code is a common performance problem, due to inability of GCD to<br>
+// properly handle QoS when a combination of queues and semaphors is used.<br>
+// Good code would either use asynchronous API (when available), or perform<br>
+// the necessary action in asynchronous callback.<br>
+//<br>
+// Currently, the check is performed using a simple heuristical AST pattern<br>
+// matching.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#include "ClangSACheckers.h"<br>
+#include "clang/ASTMatchers/ASTMatchFinder.h"<br>
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"<br>
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"<br>
+#include "clang/StaticAnalyzer/Core/Checker.h"<br>
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"<br>
+#include "llvm/Support/Debug.h"<br>
+<br>
+#define DEBUG_TYPE "gcdasyncsemaphorechecker"<br>
+<br>
+using namespace clang;<br>
+using namespace ento;<br>
+using namespace ast_matchers;<br>
+<br>
+namespace {<br>
+<br>
+const char *WarningBinding = "semaphore_wait";<br>
+<br>
+class GCDAsyncSemaphoreChecker : public Checker<check::ASTCodeBody> {<br>
+public:<br>
+ void checkASTCodeBody(const Decl *D,<br>
+ AnalysisManager &AM,<br>
+ BugReporter &BR) const;<br>
+};<br>
+<br>
+class Callback : public MatchFinder::MatchCallback {<br>
+ BugReporter &BR;<br>
+ const GCDAsyncSemaphoreChecker *C;<br>
+ AnalysisDeclContext *ADC;<br>
+<br>
+public:<br>
+ Callback(BugReporter &BR,<br>
+ AnalysisDeclContext *ADC,<br>
+ const GCDAsyncSemaphoreChecker *C) : BR(BR), C(C), ADC(ADC) {}<br>
+<br>
+ virtual void run(const MatchFinder::MatchResult &Result) override;<br>
+};<br>
+<br>
+auto callsName(const char *FunctionName)<br>
+ -> decltype(callee(functionDecl())) {<br>
+ return callee(functionDecl(hasName(FunctionName)));<br>
+}<br>
+<br>
+auto equalsBoundArgDecl(int ArgIdx, const char *DeclName)<br>
+ -> decltype(hasArgument(0, expr())) {<br>
+ return hasArgument(ArgIdx, ignoringParenCasts(declRefExpr(<br>
+ to(varDecl(equalsBoundNode(DeclName))))));<br>
+}<br>
+<br>
+auto bindAssignmentToDecl(const char *DeclName) -> decltype(hasLHS(expr())) {<br>
+ return hasLHS(ignoringParenImpCasts(<br>
+ declRefExpr(to(varDecl().bind(DeclName)))));<br>
+}<br>
+<br>
+void GCDAsyncSemaphoreChecker::checkASTCodeBody(const Decl *D,<br>
+ AnalysisManager &AM,<br>
+ BugReporter &BR) const {<br>
+<br>
+ // The pattern is very common in tests, and it is OK to use it there.<br>
+ if (const auto* ND = dyn_cast<NamedDecl>(D))<br>
+ if (ND->getName().startswith("test"))<br>
+ return;<br>
+<br>
+ const char *SemaphoreBinding = "semaphore_name";<br>
+ auto SemaphoreCreateM = callExpr(callsName("dispatch_semaphore_create"));<br>
+<br>
+ auto SemaphoreBindingM = anyOf(<br>
+ forEachDescendant(<br>
+ varDecl(hasDescendant(SemaphoreCreateM)).bind(SemaphoreBinding)),<br>
+ forEachDescendant(binaryOperator(bindAssignmentToDecl(SemaphoreBinding),<br>
+ hasRHS(SemaphoreCreateM))));<br>
+<br>
+ auto SemaphoreWaitM = forEachDescendant(<br>
+ callExpr(<br>
+ allOf(<br>
+ callsName("dispatch_semaphore_wait"),<br>
+ equalsBoundArgDecl(0, SemaphoreBinding)<br>
+ )<br>
+ ).bind(WarningBinding));<br>
+<br>
+ auto AcceptsBlockM =<br>
+ forEachDescendant(callExpr(hasAnyArgument(hasType(<br>
+ hasCanonicalType(blockPointerType())<br>
+ ))));<br>
+<br>
+ auto BlockSignallingM =<br>
+ forEachDescendant(callExpr(hasAnyArgument(hasDescendant(callExpr(<br>
+ allOf(<br>
+ callsName("dispatch_semaphore_signal"),<br>
+ equalsBoundArgDecl(0, SemaphoreBinding)<br>
+ ))))));<br>
+<br>
+ auto FinalM = compoundStmt(<br>
+ SemaphoreBindingM,<br>
+ SemaphoreWaitM,<br>
+ AcceptsBlockM,<br>
+ BlockSignallingM);<br>
+<br>
+ MatchFinder F;<br>
+ Callback CB(BR, AM.getAnalysisDeclContext(D), this);<br>
+<br>
+ F.addMatcher(FinalM, &CB);<br>
+ F.match(*D->getBody(), AM.getASTContext());<br>
+}<br>
+<br>
+void Callback::run(const MatchFinder::MatchResult &Result) {<br>
+ const auto *SW = Result.Nodes.getNodeAs<CallExpr>(WarningBinding);<br>
+ assert(SW);<br>
+ BR.EmitBasicReport(<br>
+ ADC->getDecl(), C,<br>
+ /*Name=*/"Semaphore performance anti-pattern",<br>
+ /*Category=*/"Performance",<br>
+ "Possible semaphore performance anti-pattern: wait on a semaphore "<br>
+ "signalled to in a callback",<br>
+ PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC),<br>
+ SW->getSourceRange());<br>
+}<br>
+<br>
+}<br>
+<br>
+void ento::registerGCDAsyncSemaphoreChecker(CheckerManager &Mgr) {<br>
+ Mgr.registerChecker<GCDAsyncSemaphoreChecker>();<br>
+}<br>
<br>
Added: cfe/trunk/test/gcdasyncsemaphorechecker_test.m<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/gcdasyncsemaphorechecker_test.m?rev=326746&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/gcdasyncsemaphorechecker_test.m?rev=326746&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/gcdasyncsemaphorechecker_test.m (added)<br>
+++ cfe/trunk/test/gcdasyncsemaphorechecker_test.m Mon Mar 5 14:03:32 2018<br>
@@ -0,0 +1,169 @@<br>
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.osx.GCDAsyncSemaphore %s -fblocks -verify<br>
+//<br>
+typedef int dispatch_semaphore_t;<br>
+typedef void (^block_t)();<br>
+<br>
+dispatch_semaphore_t dispatch_semaphore_create(int);<br>
+void dispatch_semaphore_wait(dispatch_semaphore_t, int);<br>
+void dispatch_semaphore_signal(dispatch_semaphore_t);<br>
+<br>
+void func(void (^)(void));<br>
+void func_w_typedef(block_t);<br>
+<br>
+int coin();<br>
+<br>
+void use_semaphor_antipattern() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+// It's OK to use pattern in tests.<br>
+// We simply match the containing function name against ^test.<br>
+void test_no_warning() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100);<br>
+}<br>
+<br>
+void use_semaphor_antipattern_multiple_times() {<br>
+ dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema1);<br>
+ });<br>
+ dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+<br>
+ dispatch_semaphore_t sema2 = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema2);<br>
+ });<br>
+ dispatch_semaphore_wait(sema2, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void use_semaphor_antipattern_multiple_wait() {<br>
+ dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema1);<br>
+ });<br>
+ // FIXME: multiple waits on same semaphor should not raise a warning.<br>
+ dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+ dispatch_semaphore_wait(sema1, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void warn_incorrect_order() {<br>
+ // FIXME: ASTMatchers do not allow ordered matching, so would match even<br>
+ // if out of order.<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+}<br>
+<br>
+void warn_w_typedef() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ func_w_typedef(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void warn_nested_ast() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ if (coin()) {<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ } else {<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ }<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void use_semaphore_assignment() {<br>
+ dispatch_semaphore_t sema;<br>
+ sema = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void use_semaphore_assignment_init() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+ sema = dispatch_semaphore_create(1);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+void differentsemaphoreok() {<br>
+ dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);<br>
+ dispatch_semaphore_t sema2 = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema1);<br>
+ });<br>
+ dispatch_semaphore_wait(sema2, 100); // no-warning<br>
+}<br>
+<br>
+void nosignalok() {<br>
+ dispatch_semaphore_t sema1 = dispatch_semaphore_create(0);<br>
+ dispatch_semaphore_wait(sema1, 100);<br>
+}<br>
+<br>
+void nowaitok() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+}<br>
+<br>
+void noblockok() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+ dispatch_semaphore_signal(sema);<br>
+ dispatch_semaphore_wait(sema, 100);<br>
+}<br>
+<br>
+void storedblockok() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+ block_t b = ^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ };<br>
+ dispatch_semaphore_wait(sema, 100);<br>
+}<br>
+<br>
+void passed_semaphore_ok(dispatch_semaphore_t sema) {<br>
+ func(^{<br>
+ dispatch_semaphore_signal(sema);<br>
+ });<br>
+ dispatch_semaphore_wait(sema, 100);<br>
+}<br>
+<br>
+void warn_with_cast() {<br>
+ dispatch_semaphore_t sema = dispatch_semaphore_create(0);<br>
+<br>
+ func(^{<br>
+ dispatch_semaphore_signal((int)sema);<br>
+ });<br>
+ dispatch_semaphore_wait((int)sema, 100); // expected-warning{{Possible semaphore performance anti-pattern}}<br>
+}<br>
+<br>
+<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>