[PATCH] D101041: [analyzer] Find better description for tracked symbolic values

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.

For me it looks good now.

The changes, however, seem to be unrelated to retain count checker. Do you think it would make sense to upstream this patch independently from the other 3 patches you have in your stack? That might require some changes in the test files though.



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1532
+      // Telling the user that the value of 'a' is assigned to 'c', while
+      // correct, can be confusing.
+      StoreManager::FindUniqueBinding FB(V.getAsLocSymbol());
----------------
vsavchenko wrote:
> martong wrote:
> > vsavchenko wrote:
> > > martong wrote:
> > > > So here, we have two or three bindings attached to `c`? `foo(b)` and `a` (and `b` as well) ?
> > > > What is the guarantee that `FindUniqueBinding` will return with the correct one `foo(b)`? Seems like we iterate through an ImmutableMap (AVL tree) with `MemRegion*` keys. And the order of the iteration depends on pointer values. What I want to express, is that I don't see how do we find the correct binding, seems like we just find one, which might be the one we look for if we are lucky.
> > > > 
> > > > Perhaps we could have a lit test for this example?
> > > Good idea, I should a test for that!
> > > `FindUniqueBinding` does not only have a region, it also carries a flag checking if there are more than one binding.  `operator bool` then checks for both, thus guaranteeing that we won't choose random binding out of more than 1 - we won't choose at all.
> > Yeah, I missed that about `FindUniqueBinding`. 
> > 
> > It's just side questions then: Could we handle multiple bindings by finding the 'last' binding and tracking that back?
> > And in this example, what are the bindings for `c`?
> Can you please elaborate on what you mean by 'last', like 'last used'?
> As for `c`, you can see above that I'm looking for a node where 'c' is not bound to that value.  So, we start with the node where 'a', 'b', and 'c' all bound to symbolic value... let's say 'x'. I go up the graph to the point where 'c' is not bound to 'x' and there I look for unique binding for 'x'.  In that example, we won't find it because it has two: 'a' and 'b'.
Thanks for the explanation!

> Can you please elaborate on what you mean by 'last', like 'last used'? 

Yes, I meant the 'last used', in this case it should be `foo(b)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101041