[PATCH] D66716: [analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Do you think its ever okay to find a last store in a `BlockEdge`?

*In a `BlockEntrance` into an empty block (that has no elements but does have a terminator). At least that's what your code is fixing.

Attaching store notes to such program points is most likely not ok. We can always make ourselves a `WideningPoint` for that specific purpose, or maybe even `PreWidening` / `PostWidening`. Program points are not set in stone, we are free to make as many kinds of them as we need (which is why we also have tags).

Being able to attach an event note at all to such program point is most likely ok. Any program point can potentially represent an interesting event. So this is anyway a good change. I'd love to have some more careful testing, maybe a unittest (or somehow trick -verify with line breaks), so that to know where exactly does the note go in this case (is it the jump from the bottom of the loop? is it jump from increment to condition?). This way we'll make sure that the implementation is correct.

I'd still want you to figure out why is this widening-specific. Do i understand correctly that we're doing widening in an inappropriate moment of time? I'd prefer to have this confirmed. Or can we reproduce this crash / incorrect behavior / false positive without widening?



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1805
     // the only thing that leads to is the addition of calls to operator!=.
-    if (isa<CXXForRangeStmt>(NB->getTerminator()))
       return nullptr;
----------------
Why did this even compile? I thought i deleted these conversions in D61814 >.<


================
Comment at: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp:774-775
+
+    return PathDiagnosticLocation(
+        BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
----------------
What exactly is the terminator here in your case? Is it `NullStmt`? Is there always a terminator and/or a terminator statement?

What if that's a `BlockEntrance` to the exit-block? (do we even make such program points? i hope we don't)

I think this code needs comments in order to explain what picture do we have in mind.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66716/new/

https://reviews.llvm.org/D66716