NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Folks, I'm glad you caught it! This is a classic mistake to make with
`addTransition()` APIs.
I wish we had better APIs that don't have this problem, but instead make it
very clear how many execution paths does the checker callback *intend* to
create. Eg.,
- `C.addStateUpdate(State)` would "chain" nodes by default if called multiple
times, like you did in this patch;
- `C.addStateSplit(State1, Tag1, State2, Tag2, ..., StateN, TagN)` would add at
most N nodes (some may merge) and can be called only once per checker callback,
otherwise it traps with assertion failure;
- Similarly, mixing `C.addStateUpdate()` and `C.addStateSplit()` in the same
checker callback will trap with assertion failure ("Make up your mind, are you
trying to split the path or not?!");
- Then we can have a Swiss-Army-knife function `C.addArbitraryTransition(Pred,
State, Tag)` for all other use cases which *requires* you to specify the
predecessor manually even if it's just `C.getPredecessor()`.
================
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:
> > 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.)
I think you're right, even though technically it's always possible to make all
updates in a single transition, in practice it often leads to annoying
architectural problems. It's nice to have separation of concerns between
different parts of checker code, and "chaining" nodes together is a neat way to
achieve that.
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
