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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added inline comments.


================
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) {
----------------
MTC wrote:
> dcoughlin wrote:
> > MTC wrote:
> > > szepet wrote:
> > > > 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.
> > > Yes, it would be better to use the location of the TerminatorCondition :D.
> > Thanks for looking into fixing this.
> > 
> > I don't think using the terminator condition of the entered block is the right thing to do. The terminator is at the *end* of a basic block and this program point represents the entrance to the block. I think it is better to use the location corresponding to the first element in in the entered block.
> Thank you, dcoughlin!
> 
> I have updated the diff.  The first element  kind I can think of is only `Stmt` and `NewAllocator`. I don't know if it's enough? 
> 
It would be good to add an llvm_unreachable() assertion expressing this expectation so that we'll get an assertion failure if this turns out not to be enough. (Or if we add a new CFGElement kind in the future).


================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:695
+    CFGElement BlockFront = BE->getBlock()->front();
+    if (BlockFront.getKind() == CFGElement::Kind::Statement) {
+      return PathDiagnosticLocation(
----------------
You can use `getAs<>` in the `if` guard condition to make this more concise:

```
if (auto StmtElt = BlockFront.getAs<CFGStmt>()) {
  return PathDiagnosticLocation(StmtElt->getStmt()->getLocStart(), SMng);
}
```


https://reviews.llvm.org/D37187