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

[Balázs Kéri via Phabricator via cfe-commits]

balazske 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);
----------------
Szelethus wrote:
> Szelethus wrote:
> > 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?
> My thinking was that each checker, unless it does state splits, should really only create a single node per callback, right? The new state, however many changes it contains, should be added all at once in the single callback, no?
The problem is that multiple NoteTags are added. It is only possible to add a single NoteTag in a single transition. This is why in line 969 (in the currently shown code at time of this comment) `addTransition` is used for every new `SuccessSt` (otherwise `NewState` could be used without `NewNode`). Or is there a possibility for multiple NoteTags at one transition, or can such a feature be added? (But if the other state add functions all have a version that accepts a predecessor node, why is `generateErrorNode` exception?) (This state apply loop was changed in the recent time at least once.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137722