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:
> > 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).
> That is what I am trying to to: `Post*` for modelling and `Pre*` for
> checking. However, this `PreStmt<CXXOperatorCallExpr>()` is special since I
> have to move the arguments to the context of the called operator //before//
> the call.
Ah, it's that thing, `PreCall` then?
https://reviews.llvm.org/D32747
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
