[PATCH] D103618: [analyzer] Change FindLastStoreBRVisitor to use Tracker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I know I'm late to the party, and am just mostly thinking aloud. Awesome patch series!



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:331-341
+/// Visitor that tracks expressions and values.
+class TrackingBugReporterVisitor : public BugReporterVisitor {
+private:
+  TrackerRef ParentTracker;
+
+public:
+  TrackingBugReporterVisitor(TrackerRef ParentTracker)
----------------
Maybe we could draw more attention to the fact this is a major component of bug report construction? Dividers like this maybe:

```
//===----------------------------------------------------------------------===//
// Visitor core components.
//===----------------------------------------------------------------------===//

class BugReporterVisitor {};
// ...

class TrackingBugReporterVisitor {};

//===----------------------------------------------------------------------===//
// Specific visitors.
//===----------------------------------------------------------------------===//
```


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:340
+
+  Tracker &getParentTracker() { return *ParentTracker; }
+};
----------------
Why is this a //parent// tracker? Could visitors have their own child trackers? 


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:367-368
+/// \param Origin Only adds notes when the last store happened in a
+///        different stackframe to this one. Disregarded if the tracking kind
+///        is thorough.
+///        This is useful, because for non-tracked regions, notes about
----------------
vsavchenko wrote:
> Szelethus wrote:
> > Different or nested? (D64272)
> I'm not sure I understand this question, but it is simply a refactoring, all parameters come from previous interfaces.
Oh well, then its likely me that made this mistake :^)

Both `f`'s and `h`'s stack frame is different to `g`'s, but only `f` is nested. As a top level function, the bug path would start from `h`, so arrows and function call events would be present in it. `f` may be pruned (no diagnostic pieces would point to it), if the analyzer decides that nothing interesting happens in it (not this, but maybe a similar example). Following this logic, we're looking for nested stack frames in particular, to tell the analyzer not to prune them.
```lang=c++
void f(int **p) {
  *p = NULL; // At the point of assignment, the stackframe of f is nested in g's stackframe
}

void h() {
  *p = malloc(...); 
  g(p); // h calls g, g's stackframe is nested in h's
}

void g(int **p) {
  f(p);
  **p = 5; // warning: nullptr reference
}
```

The comment is a little dumb, as it explicitly mentions "nested" a sentence later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103618