[PATCH] D104136: [analyzer] Add better tracking for RetainCountChecker leak warnings

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp:945-947
+    ExprEngine &Eng = C.getStateManager().getOwningEngine();
+    // Let's mark this place with a special tag.
+    Tag = Eng.getDataTags().make<IdentityTag>(CE, BindReturnTo);
----------------
vsavchenko wrote:
> Szelethus wrote:
> > I don't know ObjC at all, but is this identity obvious? Do you not need a `NoteTag` to say that "Foo is an identity function, its return value equals the parameter"? Or, in the possession of the actual `PathSensitiveBugReport` that you can retrieve in the handler, you could drop a note there, maybe?
> These are usually some type of casts.  And we already provide notes when this value gets into some other region (like `'newPtr' initialized to the value of 'originalPtr'`).  I think that they are enough.  And if we want something better, I think we need to tweak that messaging (`StoreHandler` now allows that) instead of crowding user with even more notes.
Fair enough!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:953-957
+      // Let's traverse...
+      for (const ExplodedNode *N = ExprNode;
+           // ...all the nodes corresponding to the given expression...
+           N != nullptr && N->getStmtForDiagnostics() == E;
+           N = N->getFirstPred()) {
----------------
vsavchenko wrote:
> NoQ wrote:
> > I guess this part should ultimately be written in one place, eg. `ExplodedNode::findTag()` or something like that.
> > 
> > I'd also really like to explore the possibility to further limit the variety of nodes traversed here. What nodes are typically traversed here? Is it checker-tagged nodes or is it purge dead symbol nodes or something else?
> Yes, `ExplodedNode::findTag()` sounds like a great idea!
> 
> I mean it is hard to tell without calculating statistics right here and running it on a bunch of projects.  However, it is always possible to write the code that will have it the other way.  My take on it is that it is probably a mix of things.
> 
> I'd also prefer to traverse less, do you have any specific ideas here?
> I'd also really like to explore the possibility to further limit the variety of nodes traversed here. What nodes are typically traversed here? Is it checker-tagged nodes or is it purge dead symbol nodes or something else?

Is there something I'm not seeing here? Trackers basically ascend //a// path from the error node to at most the root of the ExplodedGraph (not the trimmed version, as `Tracker::track()` is called before any of that process happens), so its not much slower than `trackExpressionValue`, right?

Or does this, and likely many other future handlers run such a loop more often then what I imagine?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104136/new/

https://reviews.llvm.org/D104136