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

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

Szelethus marked 3 inline comments as done.
Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1609-1613
+  if (B->rbegin()->getKind() != CFGElement::Kind::Statement)
+    return nullptr;
+
+  // This should be the condition of the terminator block.
+  const Stmt *S = B->rbegin()->castAs<CFGStmt>().getStmt();
----------------
NoQ wrote:
> A bit clearner:
> 
> ```lang=c++
> auto StmtElem = B->rbegin().getAs<CFGStmt>();
> if (!StmtElem)
>   return nullptr;
> 
> const Stmt *S = StmtElem->getStmt();
> ```
> 
> Also how about `CFGBlock::getTerminatorCondition()`?
I peeked at it's implementation, and it seems to be incorrect.

Referencing @xazax.hun's inline:
> 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.

And this is what `CFGBlock::getTerminatorCondition()` does:
```lang=c++
Stmt *Terminator = getTerminatorStmt();
if (!Terminator)
  return nullptr;
 
Expr *E = nullptr;

switch (Terminator->getStmtClass()) {
  // ...
  case Stmt::BinaryOperatorClass: // '&&' and '||'
    E = cast<BinaryOperator>(Terminator)->getLHS();
  // ....
}
return E;
```
But nevertheless, maybe it'd be good to fix this in there and use it here.


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1640-1642
+  CFGBlock *OriginB = GetRelevantBlock(Origin);
+  if (!OriginB || !NB)
+    return nullptr;
----------------
NoQ wrote:
> `// TODO: This can be cached.`
Like, caching the `CFGBlock`s for given `ExplodedNode`s?


================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1646
+    if (const Expr *Condition = getTerminatorCondition(NB))
+      if (BR.addTrackedCondition(Condition))
+        bugreporter::trackExpressionValue(
----------------
NoQ wrote:
> All right, i still don't understand this caching based on condition expression.
> 
> You mean, like, if we're encountering the same condition multiple times (say, in a loop), we should only track it once? Why? Like, its values (which are the thing we'll really be tracking) may be different (say, on different loop iterations).
Yup, can't argue with that, I'll revise this.


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

https://reviews.llvm.org/D62883