[analyzer] Track non-zero values in ReturnVisitor

Tracking those can help to provide much better diagnostics in many cases.

In general, most of the visitor machinery should be refactored to allow
tracking the origin of arbitrary values.

rdar://36039765

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

llvm-svn: 340475

3 files changed, 52 insertions, 28 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 40e077b73c9..d9a70c38f51 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -887,15 +887,6 @@ public:
 
     RetE = RetE->IgnoreParenCasts();
 
-    // If we can't prove the return value is 0, just mark it interesting, and
-    // make sure to track it into any further inner functions.
-    if (!State->isNull(V).isConstrainedTrue()) {
-      BR.markInteresting(V);
-      ReturnVisitor::addVisitorIfNecessary(N, RetE, BR,
-                                           EnableNullFPSuppression);
-      return nullptr;
-    }
-
     // If we're returning 0, we should track where that 0 came from.
     bugreporter::trackNullOrUndefValue(N, RetE, BR, /*IsArg*/ false,
                                        EnableNullFPSuppression);
@@ -904,20 +895,33 @@ public:
     SmallString<64> Msg;
     llvm::raw_svector_ostream Out(Msg);
 
-    if (V.getAs<Loc>()) {
-      // If we have counter-suppression enabled, make sure we keep visiting
-      // future nodes. We want to emit a path note as well, in case
-      // the report is resurrected as valid later on.
-      if (EnableNullFPSuppression &&
-          Options.shouldAvoidSuppressingNullArgumentPaths())
-        Mode = MaybeUnsuppress;
+    if (State->isNull(V).isConstrainedTrue()) {
+      if (V.getAs<Loc>()) {
+
+        // If we have counter-suppression enabled, make sure we keep visiting
+        // future nodes. We want to emit a path note as well, in case
+        // the report is resurrected as valid later on.
+        if (EnableNullFPSuppression &&
+            Options.shouldAvoidSuppressingNullArgumentPaths())
+          Mode = MaybeUnsuppress;
+
+        if (RetE->getType()->isObjCObjectPointerType()) {
+          Out << "Returning nil";
+        } else {
+          Out << "Returning null pointer";
+        }
+      } else {
+        Out << "Returning zero";
+      }
 
-      if (RetE->getType()->isObjCObjectPointerType())
-        Out << "Returning nil";
-      else
-        Out << "Returning null pointer";
     } else {
-      Out << "Returning zero";
+      if (auto CI = V.getAs<nonloc::ConcreteInt>()) {
+        Out << "Returning the value " << CI->getValue();
+      } else if (V.getAs<Loc>()) {
+        Out << "Returning pointer";
+      } else {
+        Out << "Returning value";
+      }
     }
 
     if (LValue) {
@@ -1262,10 +1266,9 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
         InitE = InitE->IgnoreParenCasts();
       bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam,
                                          EnableNullFPSuppression);
-    } else {
-      ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
-                                           BR, EnableNullFPSuppression);
     }
+    ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
+                                         BR, EnableNullFPSuppression);
   }
 
   // Okay, we've found the binding. Emit an appropriate message.
diff --git a/clang/test/Analysis/diagnostics/track_subexpressions.cpp b/clang/test/Analysis/diagnostics/track_subexpressions.cpp
new file mode 100644
index 00000000000..e5a6b8ff115
--- /dev/null
+++ b/clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_analyze_cc1 -x c++ -analyzer-checker=core -analyzer-output=text -verify %s
+
+typedef unsigned char uint8_t;
+
+#define UINT8_MAX 255
+#define TCP_MAXWIN 65535
+
+uint8_t get_uint8_max() {
+  uint8_t rcvscale = UINT8_MAX; // expected-note{{'rcvscale' initialized to 255}}
+  return rcvscale; // expected-note{{Returning the value 255 (loaded from 'rcvscale')}}
+}
+
+void shift_by_undefined_value() {
+  uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
+                                // expected-note@-1{{Calling 'get_uint8_max'}}
+                                // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+                                      // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+}
diff --git a/clang/test/Analysis/uninit-const.cpp b/clang/test/Analysis/uninit-const.cpp
index f5166e6b27b..b6430307dc5 100644
--- a/clang/test/Analysis/uninit-const.cpp
+++ b/clang/test/Analysis/uninit-const.cpp
@@ -49,15 +49,17 @@ void f7(void) {
 
 
 int& f6_1_sub(int &p) {
-  return p;
+  return p; // expected-note{{Returning without writing to 'p'}}
+            // expected-note@-1{{Returning pointer (reference to 't')}}
 }
 
 void f6_1(void) {
   int t; // expected-note{{'t' declared without an initial value}}
   int p = f6_1_sub(t); //expected-warning {{Assigned value is garbage or undefined}}
-                       //expected-note@-1 {{Calling 'f6_1_sub'}}
-                       //expected-note@-2 {{Returning from 'f6_1_sub'}}
-                       //expected-note@-3 {{Assigned value is garbage or undefined}}
+                       //expected-note@-1 {{Passing value via 1st parameter 'p'}}
+                       //expected-note@-2 {{Calling 'f6_1_sub'}}
+                       //expected-note@-3 {{Returning from 'f6_1_sub'}}
+                       //expected-note@-4 {{Assigned value is garbage or undefined}}
   int q = p;
   doStuff6(q);
 }