[PATCH] D100839: [analyzer] Adjust the reported variable name in retain count checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:967-968
+
+  // While looking for the last var bindings, we can still find
+  // `AllocFirstBinding` to be one of them.  In situations like this,
+  // it would still be the easiest case to explain to our users.
----------------
vsavchenko wrote:
> NoQ wrote:
> > Given that the current state doesn't satisfy the requirement on line 945, this can happen for two reasons:
> > 1. The symbol was there when the variable died so we simply tracked it back to the last moment of time the variable was alive.
> > 2. The symbol was there but was overwritten later (and then the variable may have also, but haven't necessarily, died).
> > 
> > Case 2 is bad because we're back in business for reporting the wrong variable; it's still more rare than before the patch but i think the problem remains. Something like this should probably demonstrate it:
> > 
> > ```lang=c++
> > void foo() {
> >   Object *Original = allocate(...);
> >   Original = allocate();
> >   Original->release();
> > }
> > ```
> > 
> > A potential solution may be to add a check in `getAllVarBindingsForSymbol` that the first node in which we find our symbol corresponds to a `*PurgeDeadSymbols` program point. It's a straightforward way to find out whether we're in case 1 or in case 2.
> In this situation, we will report on line `Original = allocate();`, which should probably be a good enough indicator of what analyzer tries to say.
> In your example, we have a choice of either not report variable name at all or report `Original`
I had a look and it looks like all of our UIs are acting up quite a bit on this example. The user ends up completely convinced that it's the current value of `Original` that's getting leaked, not the original value.

Let's at least add a test and/or a comment? This definitely isn't a regression and the patch is still really good.


================
Comment at: clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist:5993
+     <key>message</key>
+     <string>Object leaked: object allocated and stored into 'New' is not referenced later in this execution path and has a retain count of +1</string>
+    </dict>
----------------
vsavchenko wrote:
> NoQ wrote:
> > This patch seems to add 3 entirely new warnings here. Why do they suddenly start showing up? Are they good?
> Because I added 3 more test cases? 😅
Uh-oh ok fair enough :D


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100839