Page MenuHomePhabricator

[analyzer] NFC: Move getStmt() and createEndOfPath() out of PathDiagnostic.
ClosedPublic

Authored by NoQ on Mon, Sep 9, 4:53 PM.

Details

Summary

These static functions deal with ExplodedNodes which is something i don't want the PathDiagnostic interface to know anything about, as it's planned to be moved out of libStaticAnalyzer.

PathDiagnosticLocation::getStmt(N), a function commonly used in bug visitors, is now known as ExplodedNode::getStmtForDiagnostics(). Because it contains a hack that allows it to avoid body-farmed locations, it should only be used for diagnostics and is useless for most other purposes, hence the name. I added a few FIXMEs in places where this may be already causing problems. PathDiagnosticLocation::getNextStmt(N) and a couple of helper functions from BugReporter.cpp now live nearby.

PathDiagnosticLocation::createEndOfPath(N) has a similar purpose but is more sophisticated and is used mostly by end-of-path visitors. I removed this function entirely in favor of using PathSensitiveBugReport::getLocation() instead, which is literally the same thing as long as N is the bug node, which is always true. This creates a certain problem in RetainCountChecker (surprise!!~) because it has its own PathSensitiveBugReport sub-class that overrides getLocation() to not be equal to end of path, but still needs to obtain the end-of-path location in the visitor. I worked around that by giving it a separate method.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Mon, Sep 9, 4:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Sep 9, 4:53 PM
Szelethus accepted this revision.Wed, Sep 11, 5:14 AM

LGTM! I think code readability improved geatly.

This creates a certain problem in RetainCountChecker (surprise!!~)

We constantly bully this checker, but still not enough :^)

clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
284 ↗(On Diff #219455)

Lets explain what UseEnd is: Whether to use getEndLoc() rather then getBeginLoc() for \p S.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
276 ↗(On Diff #219455)

Is this correct? Shouldn't we instead say that "Find the next statement that was symbolically executed on this path of execution"?

282 ↗(On Diff #219455)

before this node?

288 ↗(On Diff #219455)

before this node?

clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2185 ↗(On Diff #219455)

Why delete the assert?

clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
302–303 ↗(On Diff #219455)

But still fails...

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
565–566 ↗(On Diff #219455)

I guess didn't change much :^)

This revision is now accepted and ready to land.Wed, Sep 11, 5:14 AM
NoQ marked an inline comment as done.Wed, Sep 11, 10:58 AM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2185 ↗(On Diff #219455)

Because it's enforced by the type system these days. This is a method on PathSensitiveBugReport that always has an error node, as asserted in its constructor. I should have removed this assert in D66572.

NoQ marked an inline comment as done.Wed, Sep 11, 11:08 AM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
2185 ↗(On Diff #219455)

Previously this assert was on the generic BugReport::Profile and it was there to state that "either a report has an error node, or a manually set location". Now have a clear separation between path-sensitive and path-insensitive reports in our type system, so it doesn't make sense to enforce this alternative manually.

Also, no, it wasn't failing :)

NoQ marked 2 inline comments as done.Wed, Sep 11, 12:12 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
302–303 ↗(On Diff #219455)

Well, not everything is a statement.

clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
565–566 ↗(On Diff #219455)

Indeed :)

NoQ marked 7 inline comments as done.Wed, Sep 11, 1:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 11, 1:54 PM