[analyzer] Reference leaked object by name, even if it was created in an inlined function.

[george.karpenkov]

Summary

rdar://45532181

[NoQ]

Yup, sounds reasonable at a glance.

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

381–382

Is this comment still relevant?

clang/test/Analysis/retain-release-path-notes.m

231

initI -> initY?

clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

struct AllocationInfo {
   const LocationContext *InterestingMethodContext;
   AllocationInfo(const ExplodedNode *InN,
                  const MemRegion *InR,
                  const LocationContext *InInterestingMethodContext) :
     N(InN), R(InR), InterestingMethodContext(InInterestingMethodContext) {}
 };
 } // end anonymous namespace
 
-static AllocationInfo
-GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N,
-                  SymbolRef Sym) {
+static AllocationInfo GetAllocationSite(ProgramStateManager &StateMgr,
+                                        const ExplodedNode *N, SymbolRef Sym) {
   const ExplodedNode *AllocationNode = N;
   const ExplodedNode *AllocationNodeInCurrentOrParentContext = N;
   const MemRegion *FirstBinding = nullptr;
   const LocationContext *LeakContext = N->getLocationContext();
 
   // The location context of the init method called on the leaked object, if
   // available.
   const LocationContext *InitMethodContext = nullptr;
while (N) {
     // to it and that reference is overwritten/dropped by another call to
     // the block.
     if (NContext == LeakContext || NContext->isParentOf(LeakContext))
       AllocationNodeInCurrentOrParentContext = N;
 
     // Find the last init that was called on the given symbol and store the
     // init method's location context.
     if (!InitMethodContext)
-      if (Optional<CallEnter> CEP = N->getLocation().getAs<CallEnter>()) {
+      if (auto CEP = N->getLocation().getAs<CallEnter>()) {
         const Stmt *CE = CEP->getCallExpr();
-        if (const ObjCMessageExpr *ME = dyn_cast_or_null<ObjCMessageExpr>(CE)) {
+        if (const auto *ME = dyn_cast_or_null<ObjCMessageExpr>(CE)) {
           const Stmt *RecExpr = ME->getInstanceReceiver();
           if (RecExpr) {
             SVal RecV = St->getSVal(RecExpr, NContext);
             if (ME->getMethodFamily() == OMF_init && RecV.getAsSymbol() == Sym)
               InitMethodContext = CEP->getCalleeContext();
           }
         }
       }
 
-    N = N->pred_empty() ? nullptr : *(N->pred_begin());
+    N = N->getFirstPred();
   }
 
   // If we are reporting a leak of the object that was allocated with alloc,
   // mark its init method as interesting.
   const LocationContext *InterestingMethodContext = nullptr;
   if (InitMethodContext) {
     const ProgramPoint AllocPP = AllocationNode->getLocation();
     if (Optional<StmtPoint> SP = AllocPP.getAs<StmtPoint>())
       if (const ObjCMessageExpr *ME = SP->getStmtAs<ObjCMessageExpr>())
         if (ME->getMethodFamily() == OMF_alloc)
           InterestingMethodContext = InitMethodContext;
   }
 
   // If allocation happened in a function different from the leak node context,
   // do not report the binding.
   assert(N && "Could not find allocation node");
-  if (N->getLocationContext() != LeakContext) {
+
+  if (AllocationNodeInCurrentOrParentContext &&
+      AllocationNodeInCurrentOrParentContext->getLocationContext() !=
+          LeakContext)
     FirstBinding = nullptr;
-  }
 
   return AllocationInfo(AllocationNodeInCurrentOrParentContext,
                         FirstBinding,
                         InterestingMethodContext);
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 CFRefReportVisitor::getEndPath(BugReporterContext &BRC,
                                const ExplodedNode *EndN, BugReport &BR) {
   BR.markInteresting(Sym);
   return BugReporterVisitor::getDefaultEndPath(BRC, EndN, BR);
 }
 
 std::shared_ptr<PathDiagnosticPiece>
 CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC,
                                    const ExplodedNode *EndN, BugReport &BR) {
 
   // Tell the BugReporterContext to report cases when the tracked symbol is
   // assigned to different variables, etc.
   BR.markInteresting(Sym);
 
   // We are reporting a leak.  Walk up the graph to get to the first node where
   // the symbol appeared, and also get the first VarDecl that tracked object
   // is stored to.
-  AllocationInfo AllocI =
-    GetAllocationSite(BRC.getStateManager(), EndN, Sym);
+  AllocationInfo AllocI = GetAllocationSite(BRC.getStateManager(), EndN, Sym);
 
   const MemRegion* FirstBinding = AllocI.R;
   BR.markInteresting(AllocI.InterestingMethodContext);
 
   SourceManager& SM = BRC.getSourceManager();
 
   // Compute an actual location for the leak.  Sometimes a leak doesn't
   // occur at an actual statement (e.g., transition between blocks; end
if (RV->getKind() == RefVal::ErrorLeakReturned) {
     // FIXME: Per comments in rdar://6320065, "create" only applies to CF
     // objects.  Only "copy", "alloc", "retain" and "new" transfer ownership
     // to the caller for NS objects.
     const Decl *D = &EndN->getCodeDecl();
 
     os << (isa<ObjCMethodDecl>(D) ? " is returned from a method "
                                   : " is returned from a function ");
 
-    if (D->hasAttr<CFReturnsNotRetainedAttr>())
+    if (D->hasAttr<CFReturnsNotRetainedAttr>()) {
       os << "that is annotated as CF_RETURNS_NOT_RETAINED";
-    else if (D->hasAttr<NSReturnsNotRetainedAttr>())
+    } else if (D->hasAttr<NSReturnsNotRetainedAttr>()) {
       os << "that is annotated as NS_RETURNS_NOT_RETAINED";
-    else {
+      // TODO: once the patch is ready, insert a case for OS_RETURNS_NOT_RETAINED
+    } else {
       if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) {
         if (BRC.getASTContext().getLangOpts().ObjCAutoRefCount) {
           os << "managed by Automatic Reference Counting";
         } else {
           os << "whose name ('" << MD->getSelector().getAsString()
              << "') does not start with "
                 "'copy', 'mutableCopy', 'alloc' or 'new'."
                 "  This violates the naming convention rules"
if (PDecl && isa<ParmVarDecl>(PDecl)) {
       PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr);
       Location = ParamLocation;
       UniqueingLocation = ParamLocation;
       UniqueingDecl = Ctx.getLocationContext()->getDecl();
     }
   }
 }
 
-void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) {
+void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,
+                                          SymbolRef sym) {
   // Most bug reports are cached at the location where they occurred.
   // With leaks, we want to unique them by the location where they were
   // allocated, and only report a single path.  To do this, we need to find
   // the allocation site of a piece of tracked memory, which we do via a
   // call to GetAllocationSite.  This will walk the ExplodedGraph backwards.
   // Note that this is *not* the trimmed graph; we are guaranteed, however,
   // that all ancestor nodes that represent the allocation site have the
   // same SourceLocation.
   const ExplodedNode *AllocNode = nullptr;
 
   const SourceManager& SMgr = Ctx.getSourceManager();
 
   AllocationInfo AllocI =
       GetAllocationSite(Ctx.getStateManager(), getErrorNode(), sym);
 
   AllocNode = AllocI.N;
   AllocBinding = AllocI.R;
   markInteresting(AllocI.InterestingMethodContext);
 
   // Get the SourceLocation for the allocation site.
   // FIXME: This will crash the analyzer if an allocation comes from an
   // implicit call (ex: a destructor call).

clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist

          <key>line</key><integer>237</integer>
          <key>col</key><integer>23</integer>
          <key>file</key><integer>0</integer>
         </dict>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
-     <string>Object leaked: allocated object is not referenced later in this execution path and has a retain count of +1</string>
+     <string>Object leaked: object allocated and stored into &apos;y&apos; is not referenced later in this execution path and has a retain count of +1</string>
      <key>message</key>
-     <string>Object leaked: allocated object is not referenced later in this execution path and has a retain count of +1</string>
+     <string>Object leaked: object allocated and stored into &apos;y&apos; is not referenced later in this execution path and has a retain count of +1</string>
     </dict>
    </array>
-   <key>description</key><string>Potential leak of an object</string>
+   <key>description</key><string>Potential leak of an object stored into &apos;y&apos;</string>
    <key>category</key><string>Memory (Core Foundation/Objective-C)</string>
    <key>type</key><string>Leak</string>
    <key>check_name</key><string>osx.cocoa.RetainCount</string>
    <!-- This hash is experimental and going to change! -->
    <key>issue_hash_content_of_line_in_context</key><string>8c27524f691296551f9e52856b824326</string>
   <key>issue_context_kind</key><string>Objective-C method</string>
   <key>issue_context</key><string>test</string>
   <key>issue_hash_function_offset</key><string>8</string>

clang/test/Analysis/osobject-retain-release.cpp

struct OSObject {
   unsigned int foo() { return 42; }
 
   static OSObject *generateObject(int);
 
   static const OSMetaClass * const metaClass;
 };
 
 struct OSIterator : public OSObject {
+
+  static const OSMetaClass * const metaClass;
 };
 
 struct OSArray : public OSObject {
   unsigned int getCount();
 
   static OSArray *withCapacity(unsigned int capacity);
   static void consumeArray(OS_CONSUME OSArray * array);
 
   static OSArray* consumeArrayHasCode(OS_CONSUME OSArray * array) {
     return nullptr;
   }
 
   OSIterator * getIterator();
 
   static OS_RETURNS_NOT_RETAINED OSArray *MaskedGetter();
   static OS_RETURNS_RETAINED OSArray *getOoopsActuallyCreate();
 
-
   static const OSMetaClass * const metaClass;
 };
 
 struct OtherStruct {
   static void doNothingToArray(OSArray *array);
   OtherStruct(OSArray *arr);
 };
 
OSArray *createArray() {
     return OSArray::withCapacity(10);
   }
 
   OSArray *createArraySourceUnknown();
 
   OSArray *getArraySourceUnknown();
 };
 
+OSArray *generateArray() {
+  return OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}}
+}
+
+unsigned int check_leak_good_error_message() {
+  unsigned int out;
+  {
+    OSArray *leaked = generateArray(); // expected-note{{Calling 'generateArray'}}
+                                       // expected-note@-1{{Returning from 'generateArray'}}
+    out = leaked->getCount(); // expected-warning{{Potential leak of an object stored into 'leaked'}}
+                              // expected-note@-1{{Object leaked: object allocated and stored into 'leaked' is not referenced later in this execution path and has a retain count of +1}}
+  }
+  return out;
+}
+
+unsigned int check_leak_msg_temporary() {
+  return generateArray()->getCount();
+}
+
 void check_confusing_getters() {
   OSArray *arr = OSArray::withCapacity(10);
 
   ArrayOwner *AO = ArrayOwner::create(arr);
   AO->getArray();
 
   AO->release();
   arr->release();
if (arr) {
     return obj->foo(); // no-warning
   }
 }
 
 void check_dynamic_cast_null_branch(OSObject *obj) {
   OSArray *arr1 = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject}}
   OSArray *arr = OSDynamicCast(OSArray, obj);
   if (!arr) // expected-note{{Taking true branch}}
-    return; // expected-warning{{Potential leak}}
+    return; // expected-warning{{Potential leak of an object stored into 'arr1'}}
             // expected-note@-1{{Object leaked}}
   arr1->release();
 }
 
 void check_dynamic_cast_null_check() {
   OSArray *arr = OSDynamicCast(OSArray, OSObject::generateObject(1)); // expected-note{{Call to function 'generateObject' returns an OSObject}}
     // expected-warning@-1{{Potential leak of an object}}
     // expected-note@-2{{Object leaked}}

clang/test/Analysis/retain-release-path-notes.m

 
 +(void)test {
   // initX is inlined since we explicitly mark it as interesting
   id x = [[MyObj alloc] initX]; // expected-warning {{Potential leak of an object}}
                                 // expected-note@-1 {{Method returns an instance of MyObj with a +1 retain count}}
                                 // expected-note@-2 {{Calling 'initX'}}
                                 // expected-note@-3 {{Returning from 'initX'}}
                                 // expected-note@-4 {{Object leaked: allocated object is not referenced later in this execution path and has a retain count of +1}}
   // initI is inlined because the allocation happens within initY
   id y = [[MyObj alloc] initY];
                                 // expected-note@-1 {{Calling 'initY'}}
                                 // expected-note@-2 {{Returning from 'initY'}}
 
   // initZ is not inlined
   id z = [[MyObj alloc] initZ]; // expected-warning {{Potential leak of an object}}
-                                // expected-note@-1 {{Object leaked: allocated object is not referenced later in this execution path and has a retain count of +1}}
+                                // expected-note@-1 {{Object leaked: object allocated and stored into 'y' is not referenced later in this execution path and has a retain count of +1}}
 
   [x release];
   [z release];
 }
 @end
 
 
 void CFOverAutorelease() {