This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Change FindLastStoreBRVisitor to use Tracker
ClosedPublic

Authored by vsavchenko on Jun 3 2021, 7:40 AM.

Details

Summary

Additionally, this commit completely removes any uses of
FindLastStoreBRVisitor from the analyzer except for the
one in Tracker.

The next step is actually removing this class altogether
from the header file.

Diff Detail

Unit TestsFailed

Event Timeline

vsavchenko created this revision.Jun 3 2021, 7:40 AM
vsavchenko requested review of this revision.Jun 3 2021, 7:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2021, 7:40 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This one is completely wrong though. Visitor can definitely outlive Tracker, unlike handlers. I need to use some reference-counting solution here.

vsavchenko updated this revision to Diff 349563.Jun 3 2021, 8:40 AM

Fix dangling reference problem

NoQ added inline comments.Jun 9 2021, 9:55 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2274

How does lifetime work here? Do I understand correctly that the tracker is only kept alive by the FindLastStoreBRVisitor instance, even after its completion?

vsavchenko added inline comments.Jun 10 2021, 1:38 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
2274

Correct, every TrackingVisitor keeps its parent tracker alive. We can optimize it a bit by setting it to null when the visitor is done early. But it feels like such a negligible gain that I didn't bother.

NoQ accepted this revision.Jun 10 2021, 11:45 PM

Got it, thanks!

This revision is now accepted and ready to land.Jun 10 2021, 11:45 PM
This revision was landed with ongoing or failed builds.Jun 11 2021, 2:52 AM
This revision was automatically updated to reflect the committed changes.
Szelethus added inline comments.Jun 14 2021, 4:27 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
362–363

Different or nested? (D64272)

vsavchenko added inline comments.Jun 14 2021, 4:48 AM
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
362–363

I'm not sure I understand this question, but it is simply a refactoring, all parameters come from previous interfaces.

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

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
326–336

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.
//===----------------------------------------------------------------------===//
335

Why is this a parent tracker? Could visitors have their own child trackers?

362–363

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.

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.