[PATCH] D58067: [Analyzer] Crash fix for FindLastStoreBRVisitor

[Artem Dergachev via Phabricator via cfe-commits]

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

Aha, right, indeed, thanks, nice!

We should eventually remove `operator==()` from the `SVal` class because it doesn't do anything you'd possibly imagine it to do. However we do need to come up with adequate alternatives. And by looking at this patch, it seems that we actually need two alternatives:

1. An attempt to figure out that two `SVal`s are equal. The result of such comparison is state-specific - eg., ($a == $b) may be true in states that follow the true-branch of "if (a == b)" and false otherwise. This is how `evalBinOp(BO_EQ)` should behave. And this sounds impossible to implement without a perfect constraint solver, so the user should always be prepared to receive an unknown answer in some cases.
2. An attempt to figure out if two `SVal`s are representing the same value, that is, you cannot replace one value with another without an assignment. This is what we need in this patch. Eg., in

  int a;
  int b;
  
  void foo() {
    if (a == b) {
      b = a;
    } 
  }

we don't mind* noticing that an assignment has happened by noticing that `$b` was replaced with `$a`. I guess the author suspected that `SVal::operator==()` does exactly that; however, as you point out, for lazy compound values this is not the case because they're "not normalized", i.e. contain more information than necessary. And in fact it's not really possible to compare two lazy compound values (even for "sameness") without a perfect constraint solver (consider a symbolic-offset binding that would or would not overlap with the parent region of the LCV, depending on constraint offsets).

As far as i understand, your approach now misses assignments to fields within the lazy compound value (because you're not checking that the corresponding stores are actually equal when restricted to the given parent region), which may probably still lead to confusing diagnostics, but that's definitely better than before.

__
*Though ideally i'd definitely prefer noticing that an assignment has happened simply by realizing that there's an assignment operator at this program point. But that's a bigger question of why is our whole visitor infrastructure so inside-out. I mean, why does it have to reverse-engineer `ExprEngine`'s logic instead of simply communicating with it somehow?...



================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:156-158
+/// Two values match if either they are equal, or for LazyCompoundVals their
+/// regions are equal and their stores are equal to the stores of their
+/// exploded nodes.
----------------
I think it's more important for a comment to explain *why* exactly does the code do whatever it does, than *what* specifically does it do. I.e., could you say something about how comparing //internal representations// of symbolic values (via `SVal::operator==()`) is a valid way to check if the value was updated (even if the new value is in fact equal to the old value), unless it's a `LazyCompoundValue` that may have a different internal representation every time it is loaded from the state? ...Which is why you do an //approximate// comparison for lazy compound values, checking that they are the immediate snapshots of the tracked region's bindings within the node's respective states but not really checking that these snapshots actually contain the same set of bindings.

Also, is it possible that these values both have the same region that is different from the region `R` we're tracking? I suspect that it isn't possible with immediate loads from the `RegionStore` as it currently is, but i wouldn't expect this to happen in the general case or be future-proof. Maybe we should assert that - either within this function or immediately after this function.


================
Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:159
+/// exploded nodes.
+bool matchesValue(const ExplodedNode *LeftNode, SVal LeftVal,
+                  const ExplodedNode *RightNode, SVal RightVal) {
----------------
baloghadamsoftware wrote:
> Maybe we should find a better name. Even better we could place this function into `LazyCompoundVal` but with 'Store` or `ProgramStateRef` parameters instead of `ExplodedNode*`.
Hmm, dunno. "`hasVisibleUpdate()`"?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58067