NoQ added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:605
+ if (Pos && !Pos->isValid()) {
+ // If I do not put a tag here, some invalidation tests will fail
+ static CheckerProgramPointTag Tag("InvalidatedIteratorChecker",
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > This needs investigation, because it may be nasty.
> >
> > `generateNonFatalErrorNode()` returns null when the exact same non-fatal
> > error node, also produced by the iterator checker with the exact same
> > program state and exact same program point and exact same tag on the
> > program point already exists. As far as i understand, the only difference
> > your tag makes is that the tag is now different, so it is not merged with
> > the existing node. However, it is worth it to try to find out why the node
> > gets merged at all.
> >
> > This may be caused by an accidental state split. For example, if you are
> > calling `generateNonFatalErrorNode()` twice in the same checker callback
> > without chaining them together (passing node returned by one as an argument
> > to another), this in fact splits states. I'm not immediately seeing such
> > places in the code - you seem to be aware of this problem and avoiding it
> > well. But still, looking at the topology of the exploded graph in the
> > failing test should help finding out what is going on.
> I made some more investigation this time. Unfortunately the case is not what
> you suggest. Only one non-fatal error node is produced. I tested it with a
> common tag (a global static so the tag is exactly the same at every
> `generateNonFatalErrorNode()`, but the tests still pass. I printed out the
> exploded graph and I found that there are indeed two nodes with the same
> state ID. The tag is the default tag automatically generated from the name of
> the checker. The first state is created in function `checkPreStatement()` for
> `CXXOperatorCallExpr` where I copy the state of the iterator from the formal
> to the actual `this` parameter. All the test fails happen at the dereference
> operator call (`*`) of another operator call (`++` or `--`). After this copy,
> when I call `generateNonFatalErrorNode()` I get `nullptr` because at some
> point under the hood (I debugged it when I created it originally) the error
> node is considered as "not new". If I use a custom tag here, the state ID
> remains, not the node ID changes.
I think i see the problem. The checker subscribes to both `PreCall` and
`PreStmt<CallExpr>` (to be exact, `CXXOperatorCallExpr`) and adds transitions
in both cases. It results with a predecessor node in `CheckerContext` that's
already tagged by the checker. Apparently this never worked, but nobody tried
that.
Ideally, we should make sure those callbacks use different program points, eg.
introduce `PreCall`/`PostCall` program point kinds and use them.
Also i wonder why are you using pre- rather than post-statement callback. You
model all other operators in `PostCall`, why did those end up here? Maybe merge
them? It is generally better to model pre-conditions and look for bugs in
`PreStmt`/`PreCall` (before we don't care what happens within the call), and
model effects in `PostStmt`/`PostCall` (because effects don't take effect until
the call happens).
https://reviews.llvm.org/D32747
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
