[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun requested changes to this revision.
xazax.hun added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:176
+  const ExplodedNode *Origin;
+  CFGControlDependencyTree ControlDepTree;
+  llvm::SmallSet<const CFGBlock *, 32> VisitedBlocks;
----------------
I think we should make clear that this visitor only operates within one function and does not track controll dependencies across functions. 


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:680
 
+} // end of anonymous namespace
+
----------------
What was the reason of adding these begin/ends? I prefer to keep such refactorings separate, also it might be controversial whether this change is desired.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1863-1864
+
+    case Stmt::ObjCForCollectionStmtClass:
+      return cast<ObjCForCollectionStmt>(S)->getCollection();
+
----------------
NoQ wrote:
> Szelethus wrote:
> > @NoQ Any feelings on this?
> This makes some sense in the long run but i think you should give up here for now. Unlike `CXXForRangeStmt`, its Objective-C counterpart doesn't mock up the AST that would have made it look like a regular for-loop, so there just isn't an AST node that would represent the part of it that corresponds to "`__begin != __end`".
> 
> Even in the C++ case, i'm not sure your current behavior would make much sense. We should probably delegate this work to a checker that knows how collections work and what makes them empty or have a certain size, something similar to what the `IteratorChecker` seems to be becoming :)
> 
> Should we give mark the report as invalid when we give up here? I've no idea, i guess i'll have to gather more empirical evidence on that.
I vaguely recall some problem with the results of getTerminatorStmt for logical operators like &&. I believe what we really want is the last expression we evaluated in the basic block which will be the last Stmt of the basic block. So if we can settle with the last stmt we can get rid of this code. 


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1899-1901
+  CFGBlock *NB = GetRelevantBlock(N);
+  if (!VisitedBlocks.insert(NB).second)
+    return nullptr;
----------------
NoQ wrote:
> I didn't really understand this part.
I guess detecting control depdendency only makes sense for the last statement of the basic block (which is the first one we see in the visitor order). But instead of maintaining a state with the visited nodes we could also check if the statement is the last one of its basic block. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D62883