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);
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137722/new/
https://reviews.llvm.org/D137722
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
