[analyzer] Prune calls to functions with linear CFGs that return a non-zero constrained value

Szelethus · Szelethus · commit 46929df72333 · 2019-08-13T23:22:33.000Z
During the evaluation of D62883, I noticed a bunch of totally meaningless notes with the pattern of "Calling 'A'" -> "Returning value" -> "Returning from 'A'", which added no value to the report at all. This patch (not only affecting tracked conditions mind you) prunes diagnostic messages to functions that return a value not constrained to be 0, and are also linear. Differential Revision: https://reviews.llvm.org/D64232 llvm-svn: 368771
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -841,7 +841,7 @@ namespace {
 /// This visitor is intended to be used when another visitor discovers that an
 /// interesting value comes from an inlined function call.
 class ReturnVisitor : public BugReporterVisitor {
-  const StackFrameContext *StackFrame;
+  const StackFrameContext *CalleeSFC;
   enum {
     Initial,
     MaybeUnsuppress,
@@ -853,10 +853,9 @@ class ReturnVisitor : public BugReporterVisitor {
   AnalyzerOptions& Options;
 
 public:
-  ReturnVisitor(const StackFrameContext *Frame,
-                bool Suppressed,
+  ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
                 AnalyzerOptions &Options)
-      : StackFrame(Frame), EnableNullFPSuppression(Suppressed),
+      : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
         Options(Options) {}
 
   static void *getTag() {
@@ -866,7 +865,7 @@ class ReturnVisitor : public BugReporterVisitor {
 
   void Profile(llvm::FoldingSetNodeID &ID) const override {
     ID.AddPointer(ReturnVisitor::getTag());
-    ID.AddPointer(StackFrame);
+    ID.AddPointer(CalleeSFC);
     ID.AddBoolean(EnableNullFPSuppression);
   }
 
@@ -950,7 +949,6 @@ class ReturnVisitor : public BugReporterVisitor {
       if (Optional<Loc> RetLoc = RetVal.getAs<Loc>())
         EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
-    BR.markInteresting(CalleeContext);
     BR.addVisitor(llvm::make_unique<ReturnVisitor>(CalleeContext,
                                                    EnableNullFPSuppression,
                                                    Options));
@@ -960,7 +958,7 @@ class ReturnVisitor : public BugReporterVisitor {
                                           BugReporterContext &BRC,
                                           BugReport &BR) {
     // Only print a message at the interesting return statement.
-    if (N->getLocationContext() != StackFrame)
+    if (N->getLocationContext() != CalleeSFC)
       return nullptr;
 
     Optional<StmtPoint> SP = N->getLocationAs<StmtPoint>();
@@ -974,7 +972,7 @@ class ReturnVisitor : public BugReporterVisitor {
     // Okay, we're at the right return statement, but do we have the return
     // value available?
     ProgramStateRef State = N->getState();
-    SVal V = State->getSVal(Ret, StackFrame);
+    SVal V = State->getSVal(Ret, CalleeSFC);
     if (V.isUnknownOrUndef())
       return nullptr;
 
@@ -1008,6 +1006,8 @@ class ReturnVisitor : public BugReporterVisitor {
     SmallString<64> Msg;
     llvm::raw_svector_ostream Out(Msg);
 
+    bool WouldEventBeMeaningless = false;
+
     if (State->isNull(V).isConstrainedTrue()) {
       if (V.getAs<Loc>()) {
 
@@ -1030,10 +1030,19 @@ class ReturnVisitor : public BugReporterVisitor {
     } 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";
+        // There is nothing interesting about returning a value, when it is
+        // plain value without any constraints, and the function is guaranteed
+        // to return that every time. We could use CFG::isLinear() here, but
+        // constexpr branches are obvious to the compiler, not necesserily to
+        // the programmer.
+        if (N->getCFG().size() == 3)
+          WouldEventBeMeaningless = true;
+
+        if (V.getAs<Loc>())
+          Out << "Returning pointer";
+        else
+          Out << "Returning value";
       }
     }
 
@@ -1052,11 +1061,20 @@ class ReturnVisitor : public BugReporterVisitor {
           Out << " (loaded from '" << *DD << "')";
     }
 
-    PathDiagnosticLocation L(Ret, BRC.getSourceManager(), StackFrame);
+    PathDiagnosticLocation L(Ret, BRC.getSourceManager(), CalleeSFC);
     if (!L.isValid() || !L.asLocation().isValid())
       return nullptr;
 
-    return std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
+    auto EventPiece = std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
+
+    // If we determined that the note is meaningless, make it prunable, and
+    // don't mark the stackframe interesting.
+    if (WouldEventBeMeaningless)
+      EventPiece->setPrunable(true);
+    else
+      BR.markInteresting(CalleeSFC);
+
+    return EventPiece;
   }
 
   PathDiagnosticPieceRef visitNodeMaybeUnsuppress(const ExplodedNode *N,
@@ -1071,7 +1089,7 @@ class ReturnVisitor : public BugReporterVisitor {
     if (!CE)
       return nullptr;
 
-    if (CE->getCalleeContext() != StackFrame)
+    if (CE->getCalleeContext() != CalleeSFC)
       return nullptr;
 
     Mode = Satisfied;
@@ -1083,7 +1101,7 @@ class ReturnVisitor : public BugReporterVisitor {
     CallEventManager &CallMgr = StateMgr.getCallEventManager();
 
     ProgramStateRef State = N->getState();
-    CallEventRef<> Call = CallMgr.getCaller(StackFrame, State);
+    CallEventRef<> Call = CallMgr.getCaller(CalleeSFC, State);
     for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) {
       Optional<Loc> ArgV = Call->getArgSVal(I).getAs<Loc>();
       if (!ArgV)
@@ -1126,7 +1144,7 @@ class ReturnVisitor : public BugReporterVisitor {
   void finalizeVisitor(BugReporterContext &, const ExplodedNode *,
                        BugReport &BR) override {
     if (EnableNullFPSuppression && ShouldInvalidate)
-      BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+      BR.markInvalid(ReturnVisitor::getTag(), CalleeSFC);
   }
 };
 
diff --git a/clang/test/Analysis/diagnostics/find_last_store.c b/clang/test/Analysis/diagnostics/find_last_store.c
@@ -2,13 +2,11 @@
 typedef struct { float b; } c;
 void *a();
 void *d() {
-  return a(); // expected-note{{Returning pointer}}
+  return a();
 }
 
 void no_find_last_store() {
-  c *e = d(); // expected-note{{Calling 'd'}}
-              // expected-note@-1{{Returning from 'd'}}
-              // expected-note@-2{{'e' initialized here}}
+  c *e = d(); // expected-note{{'e' initialized here}}
 
   (void)(e || e->b); // expected-note{{Assuming 'e' is null}}
       // expected-note@-1{{Left side of '||' is false}}
diff --git a/clang/test/Analysis/track-control-dependency-conditions.cpp b/clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -119,20 +119,18 @@ namespace variable_declaration_in_condition {
 bool coin();
 
 bool foo() {
-  return coin(); // tracking-note{{Returning value}}
+  return coin();
 }
 
 int bar;
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
-  if (int flag = foo()) // tracking-note{{Calling 'foo'}}
-                        // tracking-note@-1{{Returning from 'foo'}}
-                        // tracking-note@-2{{'flag' initialized here}}
-                        // debug-note@-3{{Tracking condition 'flag'}}
-                        // expected-note@-4{{Assuming 'flag' is not equal to 0}}
-                        // expected-note@-5{{Taking true branch}}
+  if (int flag = foo()) // tracking-note{{'flag' initialized here}}
+                        // debug-note@-1{{Tracking condition 'flag'}}
+                        // expected-note@-2{{Assuming 'flag' is not equal to 0}}
+                        // expected-note@-3{{Taking true branch}}
 
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
@@ -143,39 +141,114 @@ namespace conversion_to_bool {
 bool coin();
 
 struct ConvertsToBool {
-  operator bool() const { return coin(); } // tracking-note{{Returning value}}
+  operator bool() const { return coin(); }
 };
 
 void test() {
   int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
 
   if (ConvertsToBool())
-    // tracking-note@-1 {{Calling 'ConvertsToBool::operator bool'}}
-    // tracking-note@-2{{Returning from 'ConvertsToBool::operator bool'}}
-    // debug-note@-3{{Tracking condition 'ConvertsToBool()'}}
-    // expected-note@-4{{Assuming the condition is true}}
-    // expected-note@-5{{Taking true branch}}
+    // debug-note@-1{{Tracking condition 'ConvertsToBool()'}}
+    // expected-note@-2{{Assuming the condition is true}}
+    // expected-note@-3{{Taking true branch}}
     *x = 5; // expected-warning{{Dereference of null pointer}}
             // expected-note@-1{{Dereference of null pointer}}
 }
 
 } // end of namespace variable_declaration_in_condition
 
+namespace important_returning_pointer_loaded_from {
+bool coin();
+
+int *getIntPtr();
+
+void storeValue(int **i) {
+  *i = getIntPtr(); // tracking-note{{Value assigned to 'i'}}
+}
+
+int *conjurePointer() {
+  int *i;
+  storeValue(&i); // tracking-note{{Calling 'storeValue'}}
+                  // tracking-note@-1{{Returning from 'storeValue'}}
+  return i;       // tracking-note{{Returning pointer (loaded from 'i')}}
+}
+
+void f(int *ptr) {
+  if (ptr) // expected-note{{Assuming 'ptr' is null}}
+           // expected-note@-1{{Taking false branch}}
+    ;
+  if (!conjurePointer())
+    // tracking-note@-1{{Calling 'conjurePointer'}}
+    // tracking-note@-2{{Returning from 'conjurePointer'}}
+    // debug-note@-3{{Tracking condition '!conjurePointer()'}}
+    // expected-note@-4{{Assuming the condition is true}}
+    // expected-note@-5{{Taking true branch}}
+    *ptr = 5; // expected-warning{{Dereference of null pointer}}
+              // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace important_returning_pointer_loaded_from
+
+namespace unimportant_returning_pointer_loaded_from {
+bool coin();
+
+int *getIntPtr();
+
+int *conjurePointer() {
+  int *i = getIntPtr(); // tracking-note{{'i' initialized here}}
+  return i;             // tracking-note{{Returning pointer (loaded from 'i')}}
+}
+
+void f(int *ptr) {
+  if (ptr) // expected-note{{Assuming 'ptr' is null}}
+           // expected-note@-1{{Taking false branch}}
+    ;
+  if (!conjurePointer())
+    // tracking-note@-1{{Calling 'conjurePointer'}}
+    // tracking-note@-2{{Returning from 'conjurePointer'}}
+    // debug-note@-3{{Tracking condition '!conjurePointer()'}}
+    // expected-note@-4{{Assuming the condition is true}}
+    // expected-note@-5{{Taking true branch}}
+    *ptr = 5; // expected-warning{{Dereference of null pointer}}
+              // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace unimportant_returning_pointer_loaded_from
+
+namespace unimportant_returning_pointer_loaded_from_through_cast {
+
+void *conjure();
+
+int *cast(void *P) {
+  return static_cast<int *>(P);
+}
+
+void f() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (cast(conjure()))
+    // tracking-note@-1{{Passing value via 1st parameter 'P'}}
+    // debug-note@-2{{Tracking condition 'cast(conjure())'}}
+    // expected-note@-3{{Assuming the condition is false}}
+    // expected-note@-4{{Taking false branch}}
+    return;
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+          // expected-note@-1{{Dereference of null pointer}}
+}
+
+} // end of namespace unimportant_returning_pointer_loaded_from_through_cast
+
 namespace unimportant_returning_value_note {
 bool coin();
 
-bool flipCoin() { return coin(); } // tracking-note{{Returning value}}
+bool flipCoin() { return coin(); }
 
 void i(int *ptr) {
   if (ptr) // expected-note{{Assuming 'ptr' is null}}
            // expected-note@-1{{Taking false branch}}
     ;
   if (!flipCoin())
-    // tracking-note@-1{{Calling 'flipCoin'}}
-    // tracking-note@-2{{Returning from 'flipCoin'}}
-    // debug-note@-3{{Tracking condition '!flipCoin()'}}
-    // expected-note@-4{{Assuming the condition is true}}
-    // expected-note@-5{{Taking true branch}}
+    // debug-note@-1{{Tracking condition '!flipCoin()'}}
+    // expected-note@-2{{Assuming the condition is true}}
+    // expected-note@-3{{Taking true branch}}
     *ptr = 5; // expected-warning{{Dereference of null pointer}}
               // expected-note@-1{{Dereference of null pointer}}
 }
@@ -207,6 +280,36 @@ void i(int *ptr) {
 }
 } // end of namespace important_returning_value_note
 
+namespace important_returning_value_note_in_linear_function {
+bool coin();
+
+struct super_complicated_template_hackery {
+  static constexpr bool value = false;
+};
+
+bool flipCoin() {
+  if (super_complicated_template_hackery::value)
+    // tracking-note@-1{{'value' is false}}
+    // tracking-note@-2{{Taking false branch}}
+    return true;
+  return coin(); // tracking-note{{Returning value}}
+}
+
+void i(int *ptr) {
+  if (ptr) // expected-note{{Assuming 'ptr' is null}}
+           // expected-note@-1{{Taking false branch}}
+    ;
+  if (!flipCoin())
+    // tracking-note@-1{{Calling 'flipCoin'}}
+    // tracking-note@-2{{Returning from 'flipCoin'}}
+    // debug-note@-3{{Tracking condition '!flipCoin()'}}
+    // expected-note@-4{{Assuming the condition is true}}
+    // expected-note@-5{{Taking true branch}}
+    *ptr = 5; // expected-warning{{Dereference of null pointer}}
+              // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace important_returning_value_note_in_linear_function
+
 namespace tracked_condition_is_only_initialized {
 int getInt();
 
diff --git a/clang/test/Analysis/uninit-vals.c b/clang/test/Analysis/uninit-vals.c
@@ -149,8 +149,6 @@ int test_radar12278788_FP() {
   RetVoidFuncType f = foo_radar12278788_fp;
   return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
                                 //expected-note@-1 {{Undefined or garbage value returned to caller}}
-                                //expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-                                //expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
 }
 
 void rdar13665798() {
@@ -164,8 +162,6 @@ void rdar13665798() {
     RetVoidFuncType f = foo_radar12278788_fp;
     return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
                                   //expected-note@-1 {{Undefined or garbage value returned to caller}}
-                                  //expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-                                  //expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
   }();
 }
 
@@ -182,18 +178,14 @@ struct Point getHalfPoint() {
 void use(struct Point p); 
 
 void testUseHalfPoint() {
-  struct Point p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-                                   // expected-note@-1{{Returning from 'getHalfPoint'}}
-                                   // expected-note@-2{{'p' initialized here}}
+  struct Point p = getHalfPoint(); // expected-note{{'p' initialized here}}
   use(p); // expected-warning{{uninitialized}}
           // expected-note@-1{{uninitialized}}
 }
 
 void testUseHalfPoint2() {
   struct Point p;
-  p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-                      // expected-note@-1{{Returning from 'getHalfPoint'}}
-                      // expected-note@-2{{Value assigned to 'p'}}
+  p = getHalfPoint(); // expected-note{{Value assigned to 'p'}}
   use(p); // expected-warning{{uninitialized}}
           // expected-note@-1{{uninitialized}}
 }
