[PATCH] D103605: [analyzer] Introduce a new interface for tracking

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.



> - ... Implementation of isTracked on it's own is fairly simple. And we can have users of `isTracked` and `ExpressionHandler` at the same time. Maybe, once we migrate all uses, `ExpressionHandler` can be safely retired.

I think this sounds like a plan. More specifically, if we could maintain a //map// of all currently tracked expressions inside the `PathSensitiveBugReport` object, dynamically updated as tracking proceeds (expressions added as they're tracked and removed as soon as they're no longer necessary), with values in the map being arbitrary auxiliary information we want to pass around (eg., an explanation of how to refer to the value in notes), then all tracking machineries, regardless of how they're implemented, could communicate to each other easily through that map.

>> I still think the solution that relies on well-defined end of tracking is significantly more elegant. Which, again, could be implemented with note tags: we could have a note tag that'd call a callback stuffed directly into the bug report.
>
> I'm not sure I understood the last thing, can you please elaborate?

I was thinking of something like:

  void NullDereferenceChecker::checkLocation(...) {
    trackExpressionValue(
        BR, Ex, /*completionHandler=*/ [](const ExplodedNode *N) {
          if (N->getLocationContext()->inTopFrame())
            BR.markInvalid();
        }); // stashes completionHandler into the aforementioned tracking map
            // inside BR for Ex.
  }
  
  void ExprEngine::processBranch(...) {
    Bldr.addTransition(..., getNoteTag([](BugReport &BR) {
      if (BR.isTracked(Condition)) {
        // We've tracked a null pointer back to the assumption.
        // Our job here is done. Look up the callback in the tracking map.
        if (auto completionHandler = BR.getTrackingCompletionHandler(Condition))
          (*completionHandler)();
      }
    }));
  }



> - Why do we even need to introduce tracker and event handlers if we are going to hide/delete them in this plan? The trick is about making this whole plan manageable. One enormous refactoring is hard to plan and test. Big work can demotivate and drain if you don't see the end of it. I believe that we can make great things if we take a lot of baby steps.

I completely agree. Incremental development ftw! I guess I'll keep an eye on whether we could convert some of your newly extracted handlers to note tags immediately (but most will probably require the expression tracking map to be implemented first).

>> This code could also be moved into a checker's `checkBranchCondition()` so that to make it truly checker-specific. That would come at the cost of creating a lot of redundant exploded nodes just for the sake of attaching the tag that will probably never be used so this is rather bad, wouldn't recommend.
>
> Why not model defensive checks in the checker, so we don't make undesired assumptions in the first place?

That's pretty difficult. We can't enter (or skip) a branch without recording its respective condition. We can't continue analysis unless we enter (or skip) the branch. Once the branch has exited, we can't get rid of the constraint yet because we may encounter the same condition again and we're obliged to be consistent. Even when we've exited the nested stack frame we're still obliged to be consistent, unless we're also willing to completely drop the execution path inside the function and replace it with some form of summary. Or, you know, do summaries from the start.

----

Ok, I think I mostly understand where this is going, thanks!

I'll now dive into code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103605