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

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Wed Sep 18 15:24:26 2019
New Revision: 372269

URL: http://llvm.org/viewvc/llvm-project?rev=372269&view=rev
Log:
[analyzer] PR43102: Fix an assertion and an out-of-bounds error for diagnostic location construction

Summary:
https://bugs.llvm.org/show_bug.cgi?id=43102

In today's edition of "Is this any better now that it isn't crashing?", I'd like to show you a very interesting test case with loop widening.

Looking at the included test case, it's immediately obvious that this is not only a false positive, but also a very bad bug report in general. We can see how the analyzer mistakenly invalidated `b`, instead of its pointee, resulting in it reporting a null pointer dereference error. Not only that, the point at which this change of value is noted at is at the loop, rather then at the method call.

It turns out that `FindLastStoreVisitor` works correctly, rather the supplied explodedgraph is faulty, because `BlockEdge` really is the `ProgramPoint` where this happens.
{F9855739}
So it's fair to say that this needs improving on multiple fronts. In any case, at least the crash is gone.

Full ExplodedGraph: {F9855743}

Reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso, dcoughlin, rnkovacs, TWeaver

Subscribers: JesperAntonsson, uabelho, Ka-Ka, bjope, whisperity, szepet, a.sidorin, mikhail.ramalho, donat.nagy, dkrupp, gamesh411, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D66716

Added:
    cfe/trunk/test/Analysis/loop-widening.cpp
Modified:
    cfe/trunk/lib/Analysis/PathDiagnostic.cpp
    cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Modified: cfe/trunk/lib/Analysis/PathDiagnostic.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Analysis/PathDiagnostic.cpp?rev=372269&r1=372268&r2=372269&view=diff
==============================================================================
--- cfe/trunk/lib/Analysis/PathDiagnostic.cpp (original)
+++ cfe/trunk/lib/Analysis/PathDiagnostic.cpp Wed Sep 18 15:24:26 2019
@@ -695,14 +695,18 @@ PathDiagnosticLocation::create(const Pro
     return PathDiagnosticLocation(
         CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
   } else if (Optional<BlockEntrance> BE = P.getAs<BlockEntrance>()) {
-    CFGElement BlockFront = BE->getBlock()->front();
-    if (auto StmtElt = BlockFront.getAs<CFGStmt>()) {
-      return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
-    } else if (auto NewAllocElt = BlockFront.getAs<CFGNewAllocator>()) {
-      return PathDiagnosticLocation(
-          NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+    if (Optional<CFGElement> BlockFront = BE->getFirstElement()) {
+      if (auto StmtElt = BlockFront->getAs<CFGStmt>()) {
+        return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+      } else if (auto NewAllocElt = BlockFront->getAs<CFGNewAllocator>()) {
+        return PathDiagnosticLocation(
+            NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+      }
+      llvm_unreachable("Unexpected CFG element at front of block");
     }
-    llvm_unreachable("Unexpected CFG element at front of block");
+
+    return PathDiagnosticLocation(
+        BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
   } else if (Optional<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
     return PathDiagnosticLocation(FE->getStmt(), SMng,
                                   FE->getLocationContext());

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=372269&r1=372268&r2=372269&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Sep 18 15:24:26 2019
@@ -1438,6 +1438,7 @@ FindLastStoreBRVisitor::VisitNode(const
 
   if (!StoreSite)
     return nullptr;
+
   Satisfied = true;
 
   // If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@ TrackControlDependencyCondBRVisitor::Vis
   if (ControlDeps.isControlDependent(OriginB, NB)) {
     // We don't really want to explain for range loops. Evidence suggests that
     // the only thing that leads to is the addition of calls to operator!=.
-    if (isa<CXXForRangeStmt>(NB->getTerminator()))
+    if (llvm::isa_and_nonnull<CXXForRangeStmt>(NB->getTerminatorStmt()))
       return nullptr;
 
     if (const Expr *Condition = NB->getLastCondition()) {

Added: cfe/trunk/test/Analysis/loop-widening.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/loop-widening.cpp?rev=372269&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/loop-widening.cpp (added)
+++ cfe/trunk/test/Analysis/loop-widening.cpp Wed Sep 18 15:24:26 2019
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-config widen-loops=true \
+// RUN:   -analyzer-config track-conditions=false \
+// RUN:   -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+  void m_fn1();
+};
+bool g;
+void fn1() {
+  A a;
+  A *b = &a;
+
+  for (;;) { // expected-note{{Loop condition is true.  Entering loop body}}
+             // expected-note at -1{{Loop condition is true.  Entering loop body}}
+             // expected-note at -2{{Value assigned to 'b'}}
+             // no crash during bug report construction
+
+    g = !b;     // expected-note{{Assuming 'b' is null}}
+    b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+                // expected-note at -1{{Called C++ object pointer is null}}
+  }
+}
+} // end of namespace pr43102