This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Refactoring BugReporter.cpp P2.: Clean up the construction of bug paths and finding a valid report
ClosedPublic

Authored by Szelethus on Jul 28 2019, 2:44 PM.

Details

Summary

This patch refactors the utility functions and classes around the construction of a bug path.

At a very high level, this consists of 3 steps:

  1. For all BugReports in the same BugReportEquivClass, collect all their error nodes in a set. With that set, create a new, trimmed ExplodedGraph whose leafs are all error nodes.
  2. Until a valid report is found, construct a bug path, which is yet another ExplodedGraph, that is linear from a given error node to the root of the graph.
  3. Run all visitors on the constructed bug path. If in this process the report got invalidated, start over from step 2.

Now, to the changes within this patch:

  • Do not allow the invalidation of BugReports up to the point where the trimmed graph is constructed. Checkers shouldn't add bug reports that are known to be invalid, and should use visitors and argue about the entirety of the bug path if needed.
  • Do not calculate indices. I may be biased, but I personally find code like this the hardest to follow. I'd like to point you to one of the comments in the original code:
SmallVector<const ExplodedNode *, 32> errorNodes;
for (const auto I : bugReports) {
  if (I->isValid()) {
    HasValid = true;
    errorNodes.push_back(I->getErrorNode());
  } else {
    // Keep the errorNodes list in sync with the bugReports list.
    errorNodes.push_back(nullptr);
  }
}

Not on my watch. Instead, use a far easier to follow trick: store a pointer to the BugReport in question, not an index to it.

  • Add range iterators to ExplodedGraph's successors and predecessors, and a visitor range to BugReporter.
  • Rename TrimmedGraph to BugPathGetter. Because that is what it has always been: no sane graph type should store an iterator-like state, or have an interface not exposing a single graph-like functionalities.
  • Rename ReportGraph to BugPathInfo, because it is only a linear path with some other context.
  • Instead of having both and out and in parameter (which I think isn't ever excusable unless we use the out-param for caching), return a record object with descriptive getter methods.
  • Where descriptive names weren't sufficient, compliment the code with comments.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Jul 28 2019, 2:44 PM
Szelethus set the repository for this revision to rG LLVM Github Monorepo.
Szelethus added a project: Restricted Project.
NoQ accepted this revision.Jul 29 2019, 6:13 PM

If only somebody could explain to me why do even need trimming in the first place :/

I was only keeping it around for easier exploded graph dumps reading, but these days we can trivially replace it with a flag to exploded-graph-rewriter (i.e., combine D65345 with D64110).

This revision is now accepted and ready to land.Jul 29 2019, 6:13 PM
xazax.hun accepted this revision.Jul 29 2019, 8:30 PM

Looks good, some nits inline. I agree with Artem, we should consider omitting the trimming and document how to get something similar if desired for debugging.

Just a random idea: maybe the original purpose of the trimming was to be able to reclaim parts of the exploded graph as soon as possible?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2269 ↗(On Diff #212118)

This comment was really hard for me to understand, could you rephrase it?

2305 ↗(On Diff #212118)

Formatting might be off here?

Szelethus updated this revision to Diff 212806.Aug 1 2019, 7:13 AM
Szelethus marked 2 inline comments as done.

clang-format, rephrase the comment.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 6:55 AM