[PATCH] D75698: [analyzer][WIP] Suppress bug reports where a tracked expression's latest value change was a result of an invalidation

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D75698#1909541 <https://reviews.llvm.org/D75698#1909541>, @martong wrote:

> > While invalidation is a fundamental part of static analysis, it is unfortunately not an under-approximation (resulting in fewer but more precise paths of execution)
>
> +1 for saying this out :)


It //would have been// a correct under-approximation assuming that other parts of the static analyzer were working correctly. The reason why invalidation causes false positives is because we're making state splits on every if-statement and never join back, which causes us to explore more execution paths than we're entitled to by the presumption of no dead code. Eg.,

  class C {
    bool x;
  
    void do_not_change_x();
  
    void foo() {
      if (x) { // assuming 'x' is true
        do_not_change_x(); // 'x' is invalidated
      }
  
      // We are not entitled to a state split here
      // over the invalidated value of 'x'.
      if (x) {
        ...
      }
    }
  };

Normally such eager state splits don't cause any problems because we tell the users to "simply add an assert, the code is obscure anyway". But when invalidation kicks in, like in this example, such asserts become very ugly. I.e., in this case you'd have to do something like this to suppress the warning and you'll have to do it every time you call `do_not_change_x()` for every variable that it //doesn't// change:

  bool old_x = x;
  do_not_change_x();
  assert(x == old_x && "x changed!");
  // Also suppress unused variable warning in release builds.
  (void)old_x;

P.S. So, like, we could try to emit the warning only if we covered enough execution paths to prove that there's either dead code or the warning is true. Then we would no longer care about invalidation problems. Unfortunately, i don't have any specific suggestion of how to prove such facts for an arbitrary CFG.

P.P.S. Actually you know what, maybe we should only drop the report if the constraint over the invalidated value contradicts the constraint over the old value. That'll make things a bit more complicated and will require a visitor indeed, though hopefully not as complicated as concrete value tracking, as we're still interested in only one region at a time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75698