[PATCH] D137722: [clang][analyzer] No new nodes when bug is detected in StdLibraryFunctionsChecker.

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

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:953
     if (FailureSt && !SuccessSt) {
-      if (ExplodedNode *N = C.generateErrorNode(NewState))
+      if (ExplodedNode *N = C.generateErrorNode(NewState, NewNode))
         reportBug(Call, N, Constraint.get(), Summary, C);
----------------
balazske wrote:
> Szelethus wrote:
> > Let me know if I got this right. The reason behind `generateErrorNode` not behaving like it usually does for other checkers is because of the explicitly supplied `NewState` parameter -- in its absence, the //current// path of execution is sunk. With this parameter, a new parallel node is. Correct?
> The `NewState` only sets the state of the new error node, if it is nullptr the current state is used. A new node is always added. The other new node functions (`addTransition`, `generateNonFatalErrorNode`, `generateSink` and `addSink`) have a version that can take a predecessor node, only `generateErrorNode` did not have this (and I can not find out why part of these is called "generate" and other part "add" instead of using only "generate" or "add").
> 
> The new function is used when a node sequence `CurrentNode->A->B->ErrorNode` is needed. Without the new function it is only possible to make a `CurrentNode->ErrorNode` transition, and the following incorrect graph is created:
> ```
> CurrentNode->A->B
>           |->ErrorNode
> ```
> The code here does exactly this (before the fix), in `NewNode` a sequence of nodes is appended (like A and B above), and if then an error node is created it is added to the CurrentNode. Not this is needed here, the error node should come after B. Otherwise analysis can continue after node B (that path is invalid because a constraint violation was found).
> (The "CurrentNode" is a `Pred` value that is stored in `CheckerContext` and not changed if other nodes are added.)
I've been wondering that, especially looking at the test case. Seems like this loop runs only once, how come that new nodes are added on top of `CurrentNode` (which, in this case refers to `C.getPredecessor()`, right?)? I checked the checker's code, and I can't really see why `A` and `B` would ever appear. Isn't that a bug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722