[PATCH] D36441: Add Support for Reference Counting of Parameters on the Callee Side in RetainCountChecker

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Nice work! This looks good, with some nits and testing comments inline. Have you run this on real code? What kind of false positives do you see?



================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:1853
+
+    void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym);
+    void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym);
----------------
Let's dispense with tradition and add some doxygen comments describing what these methods do.


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2442
 
+  const DeclRegion *Region = dyn_cast<DeclRegion>(sym->getOriginRegion());
+  if (Region) {
----------------
LLVM style says to use `auto *` here rather than repeating the name of the type twice.


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2479
+  AllocStmt = PathDiagnosticLocation::getStmt(AllocNode);
+  // assert(AllocStmt && "Cannot find allocation statement");
+
----------------
If you don't want this assertion, you should remove it rather than comment it out.


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2524
+    deriveParamLocation(Ctx, sym);
+
+  createDescription(Ctx, GCEnabled, IncludeAllocationLine);
----------------
I'm worried that in the future it would be possible to  reach createDescription without the Location, UniqueingLocation, and UniqueingDecl being filled in. Can you please add an assertion for this in createDescription().


================
Comment at: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp:2683
 
+  DefaultBool PerformCalleeSideParameterChecking;
+
----------------
malhar1995 wrote:
> This might be used in the future in case callee side parameter checking is added for Core Foundation and Objective-C objects.
Let's not add this if it is not used in the code.. You should either use this to control the checking for your generalized case or remove it.


================
Comment at: test/Analysis/retain-release-inline.m:305
 
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak}}
+}
----------------
Please include the entire diagnostic text in the expected-warning{{}}. This will guarantee that future changes to the checker don't change it unexpectedly.


================
Comment at: test/Analysis/retain-release-inline.m:306
+void callee_side_parameter_checking_leak(__attribute__((annotate("rc_ownership_consumed"))) isl_basic_map *bmap) { // expected-warning {{Potential leak}}
+}
+
----------------
I'd like to see some more tests. For example, you should get a warning if you set the parameter to something else; also if you return it.

Do you have a test checking for that you don't warn when a consumed parameter is correctly released?


Repository:
  rL LLVM

https://reviews.llvm.org/D36441