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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
