From: Kristof Umann <dkszelethus@gmail.com>
Date: Thu, 14 Mar 2019 16:10:29 +0000 (+0000)
Subject: [analyzer] Fix an assertation failure for invalid sourcelocation, add a new debug... 
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=4e383db1248f747a8bdc72e474af2033c49e6ad8;p=clang

[analyzer] Fix an assertation failure for invalid sourcelocation, add a new debug checker

For a rather short code snippet, if debug.ReportStmts (added in this patch) was
enabled, a bug reporter visitor crashed:

struct h {
  operator int();
};

int k() {
  return h();
}

Ultimately, this originated from PathDiagnosticLocation::createMemberLoc, as it
didn't handle the case where it's MemberExpr typed parameter returned and
invalid SourceLocation for MemberExpr::getMemberLoc. The solution was to find
any related valid SourceLocaion, and Stmt::getBeginLoc happens to be just that.

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


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

diff --git a/docs/analyzer/developer-docs/DebugChecks.rst b/docs/analyzer/developer-docs/DebugChecks.rst
index bb2f37f339..56ce015d64 100644
--- a/docs/analyzer/developer-docs/DebugChecks.rst
+++ b/docs/analyzer/developer-docs/DebugChecks.rst
@@ -285,3 +285,10 @@ There is also an additional -analyzer-stats flag, which enables various
 statistics within the analyzer engine. Note the Stats checker (which produces at
 least one bug report per function) may actually change the values reported by
 -analyzer-stats.
+
+Output testing checkers
+=======================
+
+- debug.ReportStmts reports a warning at **every** statement, making it a very
+  useful tool for testing thoroughly bug report construction and output
+  emission.
diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 7c8a3c79da..af5edae1ca 100644
--- a/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1019,6 +1019,10 @@ def ExplodedGraphViewer : Checker<"ViewExplodedGraph">,
   HelpText<"View Exploded Graphs using GraphViz">,
   Documentation<NotDocumented>;
 
+def ReportStmts : Checker<"ReportStmts">,
+  HelpText<"Emits a warning for every statement.">,
+  Documentation<NotDocumented>;
+
 } // end "debug"
 
 
diff --git a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
index 50715f5721..63215e6bd3 100644
--- a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
+++ b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
@@ -266,3 +266,34 @@ void ento::registerExplodedGraphViewer(CheckerManager &mgr) {
 bool ento::shouldRegisterExplodedGraphViewer(const LangOptions &LO) {
   return true;
 }
+
+//===----------------------------------------------------------------------===//
+// Emits a report for every Stmt that the analyzer visits.
+//===----------------------------------------------------------------------===//
+
+namespace {
+
+class ReportStmts : public Checker<check::PreStmt<Stmt>> {
+  BuiltinBug BT_stmtLoc{this, "Statement"};
+
+public:
+  void checkPreStmt(const Stmt *S, CheckerContext &C) const {
+    ExplodedNode *Node = C.generateNonFatalErrorNode();
+    if (!Node)
+      return;
+
+    auto Report = llvm::make_unique<BugReport>(BT_stmtLoc, "Statement", Node);
+
+    C.emitReport(std::move(Report));
+  }
+};
+
+} // end of anonymous namespace
+
+void ento::registerReportStmts(CheckerManager &mgr) {
+  mgr.registerChecker<ReportStmts>();
+}
+
+bool ento::shouldRegisterReportStmts(const LangOptions &LO) {
+  return true;
+}
diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
index 13e3bcfad2..cc1e7e1798 100644
--- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -571,6 +571,8 @@ static SourceLocation getValidSourceLocation(const Stmt* S,
     } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
@@ -671,7 +673,15 @@ PathDiagnosticLocation::createConditionalColonLoc(
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
                                         const SourceManager &SM) {
-  return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  assert(ME->getMemberLoc().isValid() || ME->getBeginLoc().isValid());
+
+  // In some cases, getMemberLoc isn't valid -- in this case we'll return with
+  // some other related valid SourceLocation.
+  if (ME->getMemberLoc().isValid())
+    return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
+
+  return PathDiagnosticLocation(ME->getBeginLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
diff --git a/test/Analysis/diagnostics/invalid-srcloc-fix.cpp b/test/Analysis/diagnostics/invalid-srcloc-fix.cpp
new file mode 100644
index 0000000000..0cef5e3d0f
--- /dev/null
+++ b/test/Analysis/diagnostics/invalid-srcloc-fix.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN:   -analyzer-output=plist -o %t.plist \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.ReportStmts
+
+struct h {
+  operator int();
+};
+
+int k() {
+  return h(); // expected-warning 3 {{Statement}}
+}
diff --git a/test/Analysis/plist-html-macros.c b/test/Analysis/plist-html-macros.c
index c25346d99a..0ac79be1b9 100644
--- a/test/Analysis/plist-html-macros.c
+++ b/test/Analysis/plist-html-macros.c
@@ -3,7 +3,10 @@
 
 // RUN: rm -rf %t.dir
 // RUN: mkdir -p %t.dir
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=plist-html -o %t.dir/index.plist %s
+//
+// RUN: %clang_analyze_cc1 -o %t.dir/index.plist %s \
+// RUN:   -analyzer-checker=core -analyzer-output=plist-html
+//
 // RUN: ls %t.dir | grep '\.html' | count 1
 // RUN: grep '\.html' %t.dir/index.plist | count 1