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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus, 
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, zzheng, szepet, kristof.beyls.
Herald added a project: clang.

These static functions deal with `ExplodedNode`s 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.


Repository:
  rC Clang

https://reviews.llvm.org/D67382

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
  clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
  clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -30,7 +30,6 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
@@ -524,12 +523,12 @@
 // PathDiagnosticLocation methods.
 //===--===//
 
-static SourceLocation getValidSourceLocation(const Stmt* S,
- LocationOrAnalysisDeclContext LAC,
- bool UseEnd = false) {
+SourceLocation PathDiagnosticLocation::getValidSourceLocation(
+const Stmt *S, LocationOrAnalysisDeclContext LAC, bool UseEnd) {
   SourceLocation L = UseEnd ? S->getEndLoc() : S->getBeginLoc();
-  assert(!LAC.isNull() && "A valid LocationContext or AnalysisDeclContext should "
-  "be passed to PathDiagnosticLocation upon creation.");
+  assert(!LAC.isNull() &&
+ "A valid LocationContext or AnalysisDeclContext should be passed to "
+ "PathDiagnosticLocation upon creation.");
 
   // S might be a temporary statement that does not have a location in the
   // source code, so find an enclosing statement and use its location.
@@ -778,117 +777,6 @@
   return PathDiagnosticLocation(S, SMng, P.getLocationContext());
 }
 
-static const LocationContext *
-findTopAutosynthesizedParentContext(const LocationContext *LC) {
-  assert(LC->getAnalysisDeclContext()->isBodyAutosynthesized());
-  const LocationContext *ParentLC = LC->getParent();
-  assert(ParentLC && "We don't start analysis from autosynthesized code");
-  while (ParentLC->getAnalysisDeclContext()->isBodyAutosynthesized()) {
-LC = ParentLC;
-ParentLC = LC->getParent();
-assert(ParentLC && "We don't start analysis from autosyn

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

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! I think code readability improved geatly.

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

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




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:284
+  getValidSourceLocation(const Stmt *S, LocationOrAnalysisDeclContext LAC,
+ bool UseEnd = false);
+

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:276
+
+  /// Find the next statement that is going to be executed after this node.
+  /// Useful for explaining control flow that follows the current node.

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:282
+
+  /// Find the statement that was executed immediately before that node.
+  /// Useful when the node corresponds to a CFG block entrance.

before this node?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h:288
+
+  /// Find the statement that was executed at or immediately before that node.
+  /// Useful when any nearby statement will do.

before this node?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2185
   } else {
-assert(ErrorNode);
-hash.AddPointer(GetCurrentOrPreviousStmt(ErrorNode));

Why delete the assert?



Comment at: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp:302-303
   // Find the node's current statement in the CFG.
-  if (const Stmt *S = PathDiagnosticLocation::getStmt(this))
+  // FIXME: getStmtForDiagnostics() does nasty things in order to provide
+  // a valid statement for body farms, do we need this behavior here?
+  if (const Stmt *S = getStmtForDiagnostics())

But still fails...



Comment at: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp:565-566
 
   // FIXME: Ironically, this assert actually fails in some cases.
   //assert(L.isValid());
   return L;

I guess didn't change much :^)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67382



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2185
   } else {
-assert(ErrorNode);
-hash.AddPointer(GetCurrentOrPreviousStmt(ErrorNode));

Szelethus wrote:
> Why delete the assert?
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67382



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2185
   } else {
-assert(ErrorNode);
-hash.AddPointer(GetCurrentOrPreviousStmt(ErrorNode));

NoQ wrote:
> Szelethus wrote:
> > Why delete the assert?
> 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.
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 :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67382



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 2 inline comments as done.
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp:302-303
   // Find the node's current statement in the CFG.
-  if (const Stmt *S = PathDiagnosticLocation::getStmt(this))
+  // FIXME: getStmtForDiagnostics() does nasty things in order to provide
+  // a valid statement for body farms, do we need this behavior here?
+  if (const Stmt *S = getStmtForDiagnostics())

Szelethus wrote:
> But still fails...
Well, not everything is a statement.



Comment at: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp:565-566
 
   // FIXME: Ironically, this assert actually fails in some cases.
   //assert(L.isValid());
   return L;

Szelethus wrote:
> I guess didn't change much :^)
Indeed :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D67382



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371659: [analyzer] NFC: Move getStmt() and createEndOfPath() 
out of PathDiagnostic. (authored by dergachev, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67382?vs=219455&id=219794#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67382

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
  
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/Taint.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -165,7 +165,9 @@
 return true;
 
   while (!N->pred_empty()) {
-const Stmt *S = PathDiagnosticLocation::getStmt(N);
+// FIXME: getStmtForDiagnostics() does nasty things in order to provide
+// a valid statement for body farms, do we need this behavior here?
+const Stmt *S = N->getStmtForDiagnostics();
 if (!S) {
   N = N->getFirstPred();
   continue;
Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -308,9 +308,7 @@
 BugReporterVisitor::getDefaultEndPath(const BugReporterContext &BRC,
   const ExplodedNode *EndPathNode,
   const PathSensitiveBugReport &BR) {
-  PathDiagnosticLocation L =
-  PathDiagnosticLocation::createEndOfPath(EndPathNode);
-
+  PathDiagnosticLocation L = BR.getLocation();
   const auto &Ranges = BR.getRanges();
 
   // Only add the statement itself as a range if we didn't specify any
@@ -852,7 +850,7 @@
   /// \return Source location of right hand side of an assignment
   /// into \c RegionOfInterest, empty optional if none found.
   Optional matchAssignment(const ExplodedNode *N) {
-const Stmt *S = PathDiagnosticLocation::getStmt(N);
+const Stmt *S = N->getStmtForDiagnostics();
 ProgramStateRef State = N->getState();
 auto *LCtx = N->getLocationContext();
 if (!S)
@@ -1919,7 +1917,7 @@
 static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,
  const Expr *Inner) {
   while (N) {
-if (PathDiagnosticLocation::getStmt(N) == Inner)
+if (N->getStmtForDiagnostics() == Inner)
   return N;
 N = N->getFirstPred();
   }
Index: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -30,7 +30,6 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/None.h"
@@ -524,12 +523,12 @@
 // PathDiagnosticLocation methods.
 //===--===//
 
-static SourceLocation getValidSourceLocation(const Stmt* S,
- LocationOrAnalysisDeclContext LAC,
- bool UseEnd = false) {
-  SourceLocation L = UseEnd ? S->getEndLoc() : S->getBeginLoc();
-  assert(!LAC.isNull() && "A valid LocationContext or AnalysisDeclContext should "
-  "be passed to PathDiagnosticLocation upon creation.");