martong added a comment. In D103605#2798171 <https://reviews.llvm.org/D103605#2798171>, @NoQ wrote:
> Ok. Oof. Whoa. That's amazing. > > Let me re-iterate to make sure that my understanding of this patchset is > correct. > > **Chapter 1.** //"A is correlated with B (ρ = 0.56), given C, assuming D and > under E conditions"// > > - Two new entities are introduced: trackers and handlers. > - Trackers are attached to bug reports. A single bug report may have multiple > trackers. > - Why not have only one tracker per report? > - Handlers are attached to trackers. Each tracker can have multiple handlers > with a priority system. > - Do you envision it growing into a dependency system later? > - A subclass of visitors known as "tracking visitors" are connected to > trackers. One tracker may have multiple visitors attached to it. > - Trackers are a tiny, almost uncustomizable class. It contains is a method > called `track()` that is invoked when attention of handlers is required. > - Each tracker comes with a default set of handlers. > - The only way to customize a tracker is to add extra handlers to it. > - Handlers implement a `handle()` method invoked by the tracker every time > their attention is required in `track()`. > - Handlers may return a note from their `handle()` method. If a handler > returns a note, this note is attached to the bug report and lower-priority > handlers are not invoked at all. > - There are two major classes of handlers: expression handlers and store > handlers. Expression handlers pay attention to Environment entries, store > handlers pay attention to Region Store bindings. > > **Chapter 2.** //"For immediate release: Scientists find potential link > between A and B"// > > - Bug visitors are still responsible for discovering connections across > exploded nodes. On the contrary, `track()`/`handle()` workflow is > instantaneous. > - It sounds like this whole patchset is orthogonal to reimplementing value > tracking visitors with note tags, as long as we can pass a reference to the > appropriate tracker into the tag(?) > - Tracking starts when a checker creates a tracker and invokes `track()` on > it, which instantly causes the respective `handle()` to be invoked. > - `handle()` may attach visitors. These visitors may in turn call `track()` > as they reach a different exploded node. > - The termination condition is as usual: no further visitors attached. > - It sounds like this patchset is orthogonal to the problem of detecting > the tracking termination point(?) It does make it easier to implement > customized reactions to such termination though. > > **Chapter 3.** //"A causes B all the time. What will this means for Obama?"// > > - `trackExpressionValue()` now boils down to creation of a single tracker and > an initial invocation of `track()` on the expression which in turn causes > expression handlers to immediately react. > - Most of the old body of `trackExpressionValue()` is transformed into a > default expression handler. So it's still part of an immediate reaction, just > a bit more generalized. > - Most of the visitors that previously composed `trackExpressionValue()` are > still part of the process; they're attached by the default expression handler. > - However, instead of adding notes directly, they simply invoke `track()`. > All note-attaching work is moved into additional default handlers. > > **Chapter 4.** //"I'm wearing this to ward off A!"// > > - Basic usage in checkers is basically unchanged. They still simply call > `trackExpressionValue()` and it just works. > - Advanced usage - which is the whole point of this patchset - probably > implies adding custom handlers to the tracker created by > `trackExpressionValue()`. > - Let's consider an example. Suppose you're implementing `evalCall` for > `llvm::dyn_cast`. You could have your expression handler detect a modeled > `dyn_cast` call and implement its `handle()` method to emit a > checker-specific note and start tracking the argument. > - With this implementation such note will only be emitted if the value is > tracked back to `dyn_cast`. Not every modeled `dyn_cast` will receive a note > but only the one that, say, caused a null dereference on its return value > when it failed. > - Now, how does this interact with note tags? The idea behind note tags is > so that visitors didn't need to reverse-engineer the analysis. > - It sounds like the handler is a step back to the visitor world: it > doesn't know how exactly was `dyn_cast` modeled during analysis, so it has to > reverse-engineer what happened. > - For example, in order to find out whether `dyn_cast` succeeded or failed, > it has to explore the program state to find out which values are passed into > the cast and returned from the cast. > - Whereas a note tag would have this information accessible directly. > - Hence a suggestion. Maybe instead of having a system of handlers, allow > visitors to //ask// the tracker whether `track()` was invoked at this node on > a certain expression or region? > - I.e., polling instead of callbacks. > - This would naturally integrate with note tags. > - This does not solve the priority problem. We could try to solve it > separately by, say, enforcing at most one note per exploded node, and then > implementing a similar priority system over visitors. > - Or am I missing something about your solution that makes connection to > note tags a non-issue? Now this sounds like a plan. Valeriy, this is something that I missed for such a massive refactor... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103605/new/ https://reviews.llvm.org/D103605 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
