[PATCH] D37187: [Analyzer] Fix Bug 25609 - Assertion UNREACHABLE: 'Unexpected ProgramPoint' with widen-loops=true

[Peter Szecsi via Phabricator via cfe-commits]

szepet added a reviewer: szepet.
szepet edited subscribers, added: cfe-commits; removed: szepet.
szepet added a comment.

Hello MTC,

Thanks for working on this! I planned to add a change like this in D36690 <https://reviews.llvm.org/D36690> but it worths an individual patch (did not know it was a reported bug).
Just some thoughts:

- I think the current display information is ambiguous. If I did not know the code then I would not understand what this stands for. (That is just my opinon.)  I think Artem's "Contents of <...> are wiped" idea is better but I would go with something like this: "Invalidated previously known information on <...>". (Maybe remove the word 'previously' if its too long.) It still can be weird for the user that why this happened but at least in case of a false positive he/she can see that why this was even considered by the analyzer.
- Right now there is a "race condition" between your patch and D36690 <https://reviews.llvm.org/D36690>. So in order to avoid "conflicts" I'd ask you to add some variable changing effect to the body of the loop. For example in the last test case the variable 'num' is the one which you use to show the effect of widening. In this case a line like `num++` or `num = i` would ensure that the more precise widening invalidates it as well. Other thing is that handling loops which contains pointer operation or nested loops will be a little bit more conservative, so you these will not be widened like now. (However, test_for_bug_25609()) still should be valid.
- Please upload the diff with context (git flag: -U99999) since it is really helpful for the review.
- +1 inline comment below.



================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:694
+  } else if (Optional<BlockEntrance> BE = P.getAs<BlockEntrance>()) {
+    CFGElement BlockFront = BE->getBlock()->front();
+    if (BlockFront.getKind() == CFGElement::Statement) {
----------------
I think it would be more correct to use the location what is used in case of the BlockEdge. (So on the entranced block terminator condition.) The reason is because the BlockEntrance display message will be displayed before the message of the BlockEdge (since it is an "earlier" node in the ExplodedGraph). So it would result that if check these notes in a viewer then the earlier note would belong to the later location which could be confusing.


https://reviews.llvm.org/D37187