[analyzer] Suppress bugs whose paths go through the return of a null pointer.

This is a heuristic intended to greatly reduce the number of false
positives resulting from inlining, particularly inlining of generic,
defensive C++ methods that live in header files. The suppression is
triggered in the cases where we ask to track where a null pointer came
from, and it turns out that the source of the null pointer was an inlined
function call.

This change brings the number of bug reports in LLVM from ~1500 down to
around ~300, a much more manageable number. Yes, some true positives may
be hidden as well, but from what I looked at the vast majority of silenced
reports are false positives, and many of the true issues found by the
analyzer are still reported.

I'm hoping to improve this heuristic further by adding some exceptions
next week (cases in which a bug should still be reported).

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@164449 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index 924bbf9d1c012a54a25265ea1311cc5d86a72237..7657736c2bd2bd98dd5a1b580944786cd221fe9b 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -184,7 +184,10 @@ private:
   // Cache of the "ipa-always-inline-size" setting.
   // \sa getAlwaysInlineSize
   llvm::Optional<unsigned> AlwaysInlineSize;
-  
+
+  /// \sa shouldPruneNullReturnPaths
+  llvm::Optional<bool> PruneNullReturnPaths;
+
   /// Interprets an option's string value as a boolean.
   ///
   /// Accepts the strings "true" and "false".
@@ -226,6 +229,16 @@ public:
   /// accepts the values "true" and "false".
   bool mayInlineTemplateFunctions() const;
 
+  /// Returns whether or not paths that go through null returns should be
+  /// suppressed.
+  ///
+  /// This is a heuristic for avoiding bug reports with paths that go through
+  /// inlined functions that are more defensive than their callers.
+  ///
+  /// This is controlled by the 'suppress-null-return-paths' config option,
+  /// which accepts the values "true" and "false".
+  bool shouldPruneNullReturnPaths() const;
+
   // Returns the size of the functions (in basic blocks), which should be
   // considered to be small enough to always inline.
   //

diff --git a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
index 04eb0ad97b051ad2cb81223daa29dfc647a3666c..1ffd10576656dbef7d63d0d3344f3b14aa9b634c 100644 (file)
--- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -90,6 +90,14 @@ bool AnalyzerOptions::mayInlineObjCMethod() const {
   return *ObjCInliningMode;
 }
 
+bool AnalyzerOptions::shouldPruneNullReturnPaths() const {
+  if (!PruneNullReturnPaths.hasValue())
+    const_cast<llvm::Optional<bool> &>(PruneNullReturnPaths) =
+      getBooleanOption("suppress-null-return-paths", /*Default=*/true);
+
+  return *PruneNullReturnPaths;
+}
+
 int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) const {
   std::string OptStr = Config.lookup(Name);
   if (OptStr.empty())

diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 445500ba31d6da453a52e6534e627d182c40c986..00d7d3becfe3f8b2f6b0fee96a22ff144b1bdc91 100644 (file)
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -140,9 +140,13 @@ public:
   ReturnVisitor(const StackFrameContext *Frame)
     : StackFrame(Frame), Satisfied(false) {}
 
-  virtual void Profile(llvm::FoldingSetNodeID &ID) const {
+  static void *getTag() {
     static int Tag = 0;
-    ID.AddPointer(&Tag);
+    return static_cast<void *>(&Tag);
+  }
+
+  virtual void Profile(llvm::FoldingSetNodeID &ID) const {
+    ID.AddPointer(ReturnVisitor::getTag());
     ID.AddPointer(StackFrame);
   }
 
@@ -215,10 +219,6 @@ public:
     // Don't print any more notes after this one.
     Satisfied = true;
 
-    // Build an appropriate message based on the return value.
-    SmallString<64> Msg;
-    llvm::raw_svector_ostream Out(Msg);
-
     const Expr *RetE = Ret->getRetValue();
     assert(RetE && "Tracking a return value for a void function");
     RetE = RetE->IgnoreParenCasts();
@@ -234,7 +234,18 @@ public:
     // If we're returning 0, we should track where that 0 came from.
     bugreporter::trackNullOrUndefValue(N, RetE, BR);
 
+    // Build an appropriate message based on the return value.
+    SmallString<64> Msg;
+    llvm::raw_svector_ostream Out(Msg);
+
     if (isa<Loc>(V)) {
+      // If we are pruning null-return paths as unlikely error paths, mark the
+      // report invalid. We still want to emit a path note, however, in case
+      // the report is resurrected as valid later on.
+      ExprEngine &Eng = BRC.getBugReporter().getEngine();
+      if (Eng.getAnalysisManager().options.shouldPruneNullReturnPaths())
+        BR.markInvalid(ReturnVisitor::getTag(), StackFrame);
+
       if (RetE->getType()->isObjCObjectPointerType())
         Out << "Returning nil";
       else

diff --git a/test/Analysis/inline-plist.c b/test/Analysis/inline-plist.c
index 6c8daf5d5e9213eb9c3eb208c3844896221425a9..1ccef6c2ca7827981cf653e892a6da2c610dee5c 100644 (file)
--- a/test/Analysis/inline-plist.c
+++ b/test/Analysis/inline-plist.c
@@ -1,5 +1,5 @@
-// RUN: %clang --analyze %s -fblocks -Xanalyzer -analyzer-output=text -Xclang -verify %s
-// RUN: %clang --analyze %s -fblocks -o %t
+// RUN: %clang --analyze %s -fblocks -Xanalyzer -analyzer-output=text -Xanalyzer -analyzer-config -Xanalyzer suppress-null-return-paths=false -Xclang -verify %s
+// RUN: %clang --analyze %s -fblocks -Xanalyzer -analyzer-config -Xanalyzer suppress-null-return-paths=false -o %t
 // RUN: FileCheck -input-file %t %s
 
 // <rdar://problem/10967815>

diff --git a/test/Analysis/inlining/false-positive-suppression.c b/test/Analysis/inlining/false-positive-suppression.c
new file mode 100644 (file)
index 0000000..be485e0
--- /dev/null
+++ b/test/Analysis/inlining/false-positive-suppression.c
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify -DSUPPRESSED %s
+
+int opaquePropertyCheck(void *object);
+int coin();
+
+int *dynCastToInt(void *ptr) {
+  if (opaquePropertyCheck(ptr))
+    return (int *)ptr;
+  return 0;
+}
+
+int *dynCastOrNull(void *ptr) {
+  if (!ptr)
+    return 0;
+  if (opaquePropertyCheck(ptr))
+    return (int *)ptr;
+  return 0;
+}
+
+
+void testDynCast(void *p) {
+  int *casted = dynCastToInt(p);
+  *casted = 1;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
+}
+
+void testDynCastOrNull(void *p) {
+  int *casted = dynCastOrNull(p);
+  *casted = 1;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
+}
+
+
+void testBranch(void *p) {
+  int *casted;
+
+  // Although the report will be suppressed on one branch, it should still be
+  // valid on the other.
+  if (coin()) {
+    casted = dynCastToInt(p);
+  } else {
+    if (p)
+      return;
+    casted = (int *)p;
+  }
+
+  *casted = 1; // expected-warning {{Dereference of null pointer}}
+}
+
+void testBranchReversed(void *p) {
+  int *casted;
+
+  // Although the report will be suppressed on one branch, it should still be
+  // valid on the other.
+  if (coin()) {
+    if (p)
+      return;
+    casted = (int *)p;
+  } else {
+    casted = dynCastToInt(p);
+  }
+
+  *casted = 1; // expected-warning {{Dereference of null pointer}}
+}
+
+
+// ---------------------------------------
+// FALSE NEGATIVES (over-suppression)
+// ---------------------------------------
+
+void testDynCastOrNullOfNull() {
+  // In this case we have a known value for the argument, and thus the path
+  // through the function doesn't ever split.
+  int *casted = dynCastOrNull(0);
+  *casted = 1;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
+}
+
+void testDynCastOfNull() {
+  // In this case all paths out of the function return 0, but they are all
+  // dominated by a branch whose condition we don't know!
+  int *casted = dynCastToInt(0);
+  *casted = 1;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
+}
+

diff --git a/test/Analysis/inlining/path-notes.c b/test/Analysis/inlining/path-notes.c
index bd44f0c895f503c41cf5fdee1076f4f29ab1b2d9..763614d9aaa16fdffd064df1c075aa00231cd8fd 100644 (file)
--- a/test/Analysis/inlining/path-notes.c
+++ b/test/Analysis/inlining/path-notes.c
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text -verify %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=plist-multi-file %s -o %t.plist
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text -analyzer-config suppress-null-return-paths=false -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=plist-multi-file -analyzer-config suppress-null-return-paths=false %s -o %t.plist
 // RUN: FileCheck --input-file=%t.plist %s
 
 void zero(int **p) {

diff --git a/test/Analysis/inlining/path-notes.m b/test/Analysis/inlining/path-notes.m
index 1b67051bfaa6cec71b6403ea8b5f4953b5e42543..b15b86968270af9ca5b3f5d3489498b79b4f387a 100644 (file)
--- a/test/Analysis/inlining/path-notes.m
+++ b/test/Analysis/inlining/path-notes.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text -verify %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=plist-multi-file %s -o %t.plist
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text -analyzer-config suppress-null-return-paths=false -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=plist-multi-file -analyzer-config suppress-null-return-paths=false %s -o %t.plist
 // RUN: FileCheck --input-file=%t.plist %s
 
 @interface Test