[analyzer] Track non-zero values in ReturnVisitor

[george.karpenkov]

Summary

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

[NoQ]

I totally like where this is going.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

919 ↗

(On Diff #162067)

Dunno, the seems inconsistent here.

clang/test/Analysis/uninit-const.cpp

58–62 ↗

(On Diff #162067)

Let's arrange notes in the order in which they appear.

[NoQ]

Can we rename trackNullOrUndefValue into trackConcreteOrUndefValue now?

[george.karpenkov]

Probably not yet, I'm not sure how many other implicit nullability/undefidedness assumptions are there. But I hope eventually.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

visitNodeInitial(const ExplodedNode *N, const ExplodedNode *PrevN,
 
     // Ignore aggregate rvalues.
     if (V.getAs<nonloc::LazyCompoundVal>() ||
         V.getAs<nonloc::CompoundVal>())
       return nullptr;
 
     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);
 
     // Build an appropriate message based on the return value.
     SmallString<64> Msg;
     llvm::raw_svector_ostream Out(Msg);
 
+    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())
+        if (RetE->getType()->isObjCObjectPointerType()) {
           Out << "Returning nil";
-      else
+        } else {
           Out << "Returning null pointer";
+        }
       } else {
         Out << "Returning zero";
       }
 
+    } else {
+      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) {
       if (const MemRegion *MR = LValue->getAsRegion()) {
         if (MR->canPrintPretty()) {
           Out << " (reference to ";
           MR->printPretty(Out);
           Out << ")";
         }
       }
FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
   // came from.
   if (InitE) {
     if (V.isUndef() ||
         V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
       if (!IsParam)
         InitE = InitE->IgnoreParenCasts();
       bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam,
                                          EnableNullFPSuppression);
-    } else {
-      ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
-                                           BR, EnableNullFPSuppression);
-    }
-  }
+    }
 
   // Okay, we've found the binding. Emit an appropriate message.
   SmallString<256> sbuf;
   llvm::raw_svector_ostream os(sbuf);
 
   if (Optional<PostStmt> PS = StoreSite->getLocationAs<PostStmt>()) {
     const Stmt *S = PS->getStmt();
     const char *action = nullptr;

test/Analysis/diagnostics/track_subexpressions.cpp

+// 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'}}
+}

test/Analysis/uninit-const.cpp

 
 void f7(void) {
   int m = 3;
   doStuff6(m); // no warning
 }
 
 
 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);
 }
 
 void f6_2(void) {
   int t;       //expected-note {{'t' declared without an initial value}}
   int &p = t;
   int &s = p;