baloghadamsoftware 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", ---------------- NoQ wrote: > 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? Yes! It seems to be working. Thank you for your help! https://reviews.llvm.org/D32747 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits