<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">- cfe-dev, + cfe-commits<div class=""><br class=""></div><div class="">Hi Tobias,</div><div class=""><br class=""></div><div class="">This is a great start. Thanks for the patch!</div><div class=""><br class=""></div><div class=""><blockquote type="cite" class="">Not sure I know how to use the analyzer config flags yet. My patch<br class="">currently also still causes a segfault on Analysis/retain-release.m</blockquote><div class=""><br class=""></div><div class="">The segfault is because the diagnostics machinery expects a declaration with body for the uniquing declaration (See below).</div><br class=""><blockquote type="cite" class=""> and<br class="">changes a test case, where I am not sure it should.<br class=""><br class="">I probably need some time to get this tested and get more confidence in<br class="">what it is doing. I will then put it up for review and we can see how to<br class="">add an analyzer-config flag.</blockquote></div><div class=""><br class=""></div><div class="">You can take a look at the MallocChecker and the malloc-annotations.c test to see how analyzer-config flags work.</div><div class=""><br class=""></div><div class=""><div class="">Also, in the future, it is probably best to submit patches on Phabricator: <https://<a href="http://reviews.llvm.org" class="">reviews.llvm.org</a>> This makes it easier for multiple people to comment on the same patch.</div></div><div class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Sep 8, 2016, at 2:15 AM, Tobias Grosser <<a href="mailto:tobias@grosser.es" class="">tobias@grosser.es</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">And here a slightly more cleaned up version of my patch, just for<br class="">reference.<br class=""><br class="">Best,<br class="">Tobias<br class=""><span id="cid:1473326134.4155777.e4d978a2286c4ccf2e89cea596d5e71dec56dda8.17B71BE5@content.messagingengine.com"><0001-RetainCountChecker-Also-check-function-parameters-fo.patch></span></div></div></blockquote></div><br class=""><div class=""><blockquote type="cite" class=""><div class="">From 19654ebf904c11e3fa5d7f8c938d4a0bb863fbf0 Mon Sep 17 00:00:00 2001</div><div class="">From: Tobias Grosser <<a href="mailto:tobias@grosser.es" class="">tobias@grosser.es</a>></div><div class="">Date: Thu, 8 Sep 2016 09:38:36 +0200</div><div class="">Subject: [PATCH] RetainCountChecker: Also check function parameters for leaks</div><div class=""><br class=""></div><div class="">---</div><div class=""> lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp | 87 +++++++++++++++++++---</div><div class=""> test/Analysis/retain-release-function-parameters.m | 26 +++++++</div><div class=""> test/Analysis/retain-release-gc-only.m | 2 +-</div><div class=""> 3 files changed, 105 insertions(+), 10 deletions(-)</div><div class=""> create mode 100644 test/Analysis/retain-release-function-parameters.m</div><div class=""><br class=""></div><div class="">diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp</div><div class="">index d445e91..71035a0 100644</div><div class="">--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp</div><div class="">+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp</div><div class="">@@ -1823,6 +1823,13 @@ namespace {</div><div class=""> </div><div class=""> class CFRefLeakReport : public CFRefReport {</div><div class=""> const MemRegion* AllocBinding;</div><div class="">+ const Stmt *AllocStmt;</div></blockquote><div class=""><br class=""></div><div class="">It looks like neither of these instance variables are referred to after object construction. Do they really need to be ivars?</div><br class=""><blockquote type="cite" class=""><div class="">+</div><div class="">+ void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);</div><div class="">+ void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);</div><div class="">+ void createDescription(CheckerContext &Ctx,</div><div class="">+ bool GCEnabled, bool IncludeAllocationLine);</div><div class="">+</div><div class=""> public:</div><div class=""> CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled,</div><div class=""> const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym,</div><div class="">@@ -2388,13 +2395,25 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,</div><div class=""> return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str());</div><div class=""> }</div><div class=""> </div><div class="">-CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,</div><div class="">- bool GCEnabled, const SummaryLogTy &Log,</div><div class="">- ExplodedNode *n, SymbolRef sym,</div><div class="">- CheckerContext &Ctx,</div><div class="">- bool IncludeAllocationLine)</div></blockquote><blockquote type="cite" class=""><div class="">- : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {</div><div class="">+void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) {</div><div class="">+ const SourceManager& SMgr = Ctx.getSourceManager();</div><div class="">+</div><div class="">+ if (!sym->getOriginRegion())</div><div class="">+ return;</div><div class=""> </div></blockquote><div class=""><br class=""></div><div class="">Some style notes: In LLVM style we typically use ‘auto *’ for pointers to make it clear there is not an</div><div class="">expensive copy. For example: 'auto *PDecl = …’</div><div class=""><br class=""></div><div class=""><div class="">We also try to avoided nested indentation where possible. So, for example:</div><div class=""> if (!Region)</div><div class=""> return;</div></div><br class=""><blockquote type="cite" class=""><div class="">+ auto Region = dyn_cast<DeclRegion>(sym->getOriginRegion());</div><div class="">+ if (Region) {</div><div class="">+ auto PDecl = Region->getDecl();</div><div class="">+ if (PDecl && isa<ParmVarDecl>(PDecl)) {</div></blockquote><div class=""><br class=""></div><div class="">I think it is worth adding a comment here explaining that the location</div><div class="">of the leak is the location of the parameter annotated as consumed.</div><br class=""><blockquote type="cite" class=""><div class="">+ auto ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);</div><div class="">+ Location = ParamLocation;</div><div class="">+ UniqueingLocation = ParamLocation;</div><div class="">+ UniqueingDecl = PDecl;</div></blockquote><div class=""><br class=""></div><div class="">Rather than using PDecl here, you can use the declaration for the containing function:</div><div class=""><span class="Apple-tab-span" style="white-space:pre"> </span>UniqueingDecl = Ctx.getLocationContext()->getDecl();</div><div class=""><br class=""></div><div class="">This will avoid the crash when flushing diagnostics.</div><br class=""><blockquote type="cite" class=""><div class="">+ }</div><div class="">+ }</div><div class="">+}</div><div class="">+</div><div class="">+void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) {</div><div class=""> // Most bug reports are cached at the location where they occurred.</div><div class=""> // With leaks, we want to unique them by the location where they were</div><div class=""> // allocated, and only report a single path. To do this, we need to find</div><div class="">@@ -2418,8 +2437,12 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,</div><div class=""> // FIXME: This will crash the analyzer if an allocation comes from an</div><div class=""> // implicit call (ex: a destructor call).</div><div class=""> // (Currently there are no such allocations in Cocoa, though.)</div><div class="">- const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);</div><div class="">- assert(AllocStmt && "Cannot find allocation statement");</div><div class="">+ AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);</div><div class="">+</div><div class="">+ if (!AllocStmt) {</div><div class="">+ AllocBinding = nullptr;</div><div class="">+ return;</div><div class="">+ }</div><div class=""> </div><div class=""> PathDiagnosticLocation AllocLocation =</div><div class=""> PathDiagnosticLocation::createBegin(AllocStmt, SMgr,</div><div class="">@@ -2430,8 +2453,10 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,</div><div class=""> // leaks should be uniqued on the allocation site.</div><div class=""> UniqueingLocation = AllocLocation;</div><div class=""> UniqueingDecl = AllocNode->getLocationContext()->getDecl();</div><div class="">+}</div><div class=""> </div><div class="">- // Fill in the description of the bug.</div><div class="">+void CFRefLeakReport::createDescription(CheckerContext &Ctx,</div><div class="">+ bool GCEnabled, bool IncludeAllocationLine) {</div></blockquote><div class="">Nit: The indentation here doesn’t follow LLVM guidelines. You might find the ‘clang-format’ tool helpful to match indentation in the rest</div><div class="">of the clang codebase.</div><br class=""><blockquote type="cite" class=""><div class=""> Description.clear();</div><div class=""> llvm::raw_string_ostream os(Description);</div><div class=""> os << "Potential leak ";</div><div class="">@@ -2446,6 +2471,21 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,</div><div class=""> os << " (allocated on line " << SL.getSpellingLineNumber() << ")";</div><div class=""> }</div><div class=""> }</div><div class="">+}</div><div class="">+</div><div class="">+CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts,</div><div class="">+ bool GCEnabled, const SummaryLogTy &Log,</div><div class="">+ ExplodedNode *n, SymbolRef sym,</div><div class="">+ CheckerContext &Ctx,</div><div class="">+ bool IncludeAllocationLine)</div><div class="">+ : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) {</div><div class="">+</div></blockquote><div class=""><br class=""></div>deriveParamLocation is much, much cheaper than deriveAllocLocation (since the latter walks to the path). Can you</div><div class="">change the ordering to check for the parameter first?</div><div class=""><br class=""><blockquote type="cite" class=""><div class="">+ deriveAllocLocation(Ctx, sym);</div><div class="">+</div><div class="">+ if (!AllocBinding)</div><div class="">+ deriveParamLocation(Ctx, sym);</div><div class="">+</div><div class="">+ createDescription(Ctx, GCEnabled, IncludeAllocationLine);</div><div class=""> </div><div class=""> addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, GCEnabled, Log));</div><div class=""> }</div><div class="">@@ -2459,6 +2499,7 @@ class RetainCountChecker</div><div class=""> : public Checker< check::Bind,</div><div class=""> check::DeadSymbols,</div><div class=""> check::EndAnalysis,</div><div class="">+ check::BeginFunction,</div><div class=""> check::EndFunction,</div><div class=""> check::PostStmt<BlockExpr>,</div><div class=""> check::PostStmt<CastExpr>,</div><div class="">@@ -2646,6 +2687,7 @@ public:</div><div class=""> SymbolRef Sym, ProgramStateRef state) const;</div><div class=""> </div><div class=""> void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;</div><div class="">+ void checkBeginFunction(CheckerContext &C) const;</div><div class=""> void checkEndFunction(CheckerContext &C) const;</div><div class=""> </div><div class=""> ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym,</div><div class="">@@ -3854,6 +3896,33 @@ RetainCountChecker::processLeaks(ProgramStateRef state,</div><div class=""> </div><div class=""> return N;</div><div class=""> }</div><div class="">+void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const {</div><div class="">+ if (!Ctx.inTopFrame())</div><div class="">+ return;</div><div class="">+</div><div class="">+ const auto *LCtx = Ctx.getLocationContext();</div><div class="">+ const auto *FD = dyn_cast<FunctionDecl>(LCtx->getDecl());</div></blockquote><div class=""><br class=""></div><div class="">You’ll also need to handle ObjCMethodDecls here. They don’t derive from FunctionDecl.</div><br class=""><blockquote type="cite" class=""><div class="">+</div><div class="">+ if (!FD)</div><div class="">+ return;</div><div class="">+</div><div class="">+ ProgramStateRef state = Ctx.getState();</div><div class="">+</div><div class="">+ for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) {</div><div class="">+ const auto Param = FD->getParamDecl(idx);</div><div class="">+ auto Sym = state->getSVal(state->getRegion(Param, LCtx)).getAsSymbol();</div><div class="">+</div><div class="">+ QualType Ty = Param->getType();</div><div class="">+ if (Param->hasAttr<CFConsumedAttr>())</div></blockquote><div class=""><br class=""></div><div class="">One the design philosophies of this checker is that all the knowledge of attributes lives in</div><div class="">the RetainSummary manager, which translates attributes (and naming conventions) into function</div><div class="">summaries which can then be applied (at a call site) and checked (at a return site).</div><div class=""><br class=""></div><div class="">It would be better here to get the function summary with:</div><div class=""><br class=""></div><div class="">getSummaryManager(Ctx).getFunctionSummary(FD).</div><div class=""><br class=""></div><div class="">and then get the ArgEffect for each parameter.</div><div class=""><br class=""></div><div class="">You would then create Owned references for:</div><div class="">DecRef{Msg}{StopTrackingHard} and NotOwned references for all others.</div><div class=""><br class=""></div><div class="">It is important to only add a ref binding if the type is either coreFoundation::isCFObjectRef() or cocoa::isCocoaObjectRef(). Without this, the checker will warn about all parameter types — but ownership discipline needs to be opted in on a per-type basis. Also, isCFObjectRef tells you if the RetEffect:ObjKind should be RetEffect::CF; cocoa::isCocoaObjectRef() tells you if it is RetEffect::ObjC.</div><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class="">+ state = setRefBinding(state, Sym,</div><div class="">+ RefVal::makeOwned(RetEffect::ObjKind::CF, Ty));</div><div class="">+ else</div><div class="">+ state = setRefBinding(state, Sym,</div><div class="">+ RefVal::makeNotOwned(RetEffect::ObjKind::CF, Ty));</div><div class="">+ }</div><div class="">+</div><div class="">+ Ctx.addTransition(state);</div><div class="">+}</div><div class=""> </div><div class=""> void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const {</div><div class=""> ProgramStateRef state = Ctx.getState();</div><div class="">diff --git a/test/Analysis/retain-release-function-parameters.m b/test/Analysis/retain-release-function-parameters.m</div><div class="">new file mode 100644</div><div class="">index 0000000..6afec24</div><div class="">--- /dev/null</div><div class="">+++ b/test/Analysis/retain-release-function-parameters.m</div><div class="">@@ -0,0 +1,26 @@</div><div class="">+// RUN: %clang_cc1 -analyze -analyzer-checker=osx.cocoa.RetainCount %s</div><div class="">+</div><div class="">+struct obj;</div><div class="">+typedef struct obj obj;</div><div class="">+</div><div class="">+#define __give __attribute__((cf_returns_retained))</div><div class="">+#define __take __attribute__((cf_consumed))</div><div class="">+</div><div class="">+#define __keep</div><div class="">+</div><div class="">+__give obj *alloc();</div><div class="">+void obj_free(__take obj *o);</div><div class="">+</div><div class="">+void taken_and_freed(__take obj *o) {</div><div class="">+ obj_free(o);</div><div class="">+}</div><div class="">+</div><div class="">+void taken_and_not_freed(__take obj *o) { // expected-warning {{Potential leak of an object}}</div><div class="">+}</div><div class="">+</div><div class="">+void kept_and_not_freed(__keep obj *o) {</div><div class="">+}</div><div class="">+</div><div class="">+void kept_and_freed(__keep obj *o) {</div><div class="">+ obj_free(o); // expected-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}</div><div class="">+}</div><div class="">diff --git a/test/Analysis/retain-release-gc-only.m b/test/Analysis/retain-release-gc-only.m</div><div class="">index 26eb6e1..3d3f6e1 100644</div><div class="">--- a/test/Analysis/retain-release-gc-only.m</div><div class="">+++ b/test/Analysis/retain-release-gc-only.m</div><div class="">@@ -224,7 +224,7 @@ CFTypeRef CFMakeCollectable(CFTypeRef cf) ;</div><div class=""> </div><div class=""> static __inline__ __attribute__((always_inline)) id NSMakeCollectable(CFTypeRef </div><div class=""> cf) {</div><div class="">- return cf ? (id)CFMakeCollectable(cf) : ((void*)0);</div><div class="">+ return cf ? (id)CFMakeCollectable(cf) : ((void*)0); // expected-warning {{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}}</div><div class=""> }</div><div class=""> </div><div class=""> //===----------------------------------------------------------------------===//</div><div class="">-- </div><div class="">2.7.4</div><div class=""><br class=""></div></blockquote></div></div></body></html>