[PATCH] D39398: [CFG][Analyzer] Add LoopExit element to the CFG in more cases

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.
Herald added subscribers: dkrupp, a.sidorin, rnkovacs.

These CFGs make perfect sense to me so far, and i guess this is a must-have. Thank you!

> Note: In case of IndirectGotoStmt, it could happen that we generate LoopExit elements even for loops which is not exited by that jump. However, it does not seem to be a problem. This could result that we can not apply some more precise modeling feature (like unrolling and widening) but not any more - as I can see. (Also, this is a rare case.)

This seems a bit scary because if there's no obvious one-to-once correspondence between entrances and exits along every path, how would we know when to pop loop contexts from the stack of location contexts? Like, if we attach variable lifetime checks to the loop exit, would we need to write additional code in the analyzer to see if the variable indeed goes out of scope on the current path? Could you demonstrate how it works in a test case over https://reviews.llvm.org/D41151?



================
Comment at: include/clang/Analysis/CFG.h:172-178
 /// Represents the point where a loop ends.
 /// This element is is only produced when building the CFG for the static
 /// analyzer and hidden behind the 'cfg-loopexit' analyzer config flag.
 ///
-/// Note: a loop exit element can be reached even when the loop body was never
-/// entered.
+/// Note: whenever a loop is entered a loop exit element will be encountered
+/// after leaving it. However, a loop exit element can be reached even when the
+/// loop body was never entered.
----------------
I'd be glad to see more documentation on where does this `CFGElement` appear, when, and why:
- At the end of the block within the loop?
- At the beginning of the block after the loop?
- In its own block constructed with the sole purpose of sheltering it?
Same goes for the `CFGLoopEntrace`, i guess.


================
Comment at: test/Analysis/loopexit-cfg-output.cpp:912-914
+// CHECK:       [B5]
+// CHECK-NEXT:   Succs (1): B8
+
----------------
P.S. This is not a regression introduced by your patch, but i'm really curious what does this block even do.


https://reviews.llvm.org/D39398