[PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Devin Coughlin via cfe-commits]

dcoughlin added inline comments.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:321
@@ +320,3 @@
+    // sink, we assume that a client requesting a transition to a state that is
+    // the same as the predecessor state has made a mistake. We return the
+    // predecessor and rather than cache out.
----------------
zaks.anna wrote:
> dcoughlin wrote:
> > jordan_rose wrote:
> > > What does "has made a mistake" mean? What is the mistake and how will they discover it?
> > The mistake this guard protects against is adding a transition from a state to itself, which would normally cause a cache out. My understanding is that the whole point of this guard is to silently swallow that mistake. I found that surprising, which is why I added the comment.
> Should we add an assert here? I wonder if/how much it will get triggered.
I tried adding an assert to the inverse of the 'if' condition here. The DereferenceChecker, CallAndMessageChecker, and DynamicTypePropagation all trigger it. Added a note about this in a comment.

================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:322
@@ -290,1 +321,3 @@
+    // the same as the predecessor state has made a mistake. We return the
+    // predecessor and rather than cache out.
     if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))
----------------
xazax.hun wrote:
> As a slightly related note: is it documented anywhere what "cache out" means? Maybe it would be great to refer to that document or write it if it is not written yet.
I've defined "cache out" in the comment.

================
Comment at: lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp:53
@@ -52,3 +52,3 @@
 
-  if (ExplodedNode *N = C.addTransition()) {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!BT)
----------------
dcoughlin wrote:
> zaks.anna wrote:
> > jordan_rose wrote:
> > > zaks.anna wrote:
> > > > Can this ever fail? In some cases we just assume it won't in others we tests..
> > > > 
> > > > Maybe it only fails when we cache out?
> > > It does fail when we cache out, and I think we can still cache out if Pred has a different tag the second time around.
> > There some instances in this patch where we do not check if the returned node is null. We should be consistent. 
> Ok, I'll go through and add checks where they are missing.
There were 9 locations where checks for null error generated error nodes were missing. I added them.


http://reviews.llvm.org/D12780