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

[Szelethus]

Summary

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();
}

include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:472: clang::ento::PathDiagnosticSpotPiece::PathDiagnosticSpotPiece(const clang::ento::PathDiagnosticLocation &, llvm::StringRef, PathDiagnosticPiece::Kind, bool): Assertion `Pos.isValid() && Pos.asLocation().isValid() && "PathDiagnosticSpotPiece's must have a valid location."' failed.

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.

[Szelethus]

Added context.

[NoQ]

Very nice! Crashing diagnostic locations are a fairly common issue. That said, this still makes me wonder what sort of checker were you writing that triggered this originally :)

lib/StaticAnalyzer/Checkers/DebugCheckers.cpp

271

Technically, we don't invoke checkPreStmt for every Stmt (eg., IfStmt is omitted in favor of checkBranchCondition, IntegerLiteral is outright skipped because ExprEngine doesn't even evaluate anything at this point). Moreover, for some Stmts we more-or-less-intentionally only invoke checkPreStmt but not checkPostStmt.

Also would you eventually want to expand this checker with non-Stmt callbacks?

277

My current favorite approach is:

BuiltinBug BT_stmtLoc{this, "Statement"};

...and remove the constructor. And remove the * in your make_unique<BugReport>.

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

574–575

A normal day in Clang :)

679

I think it's worth commenting why exactly this may fail. It's fairly hard to guess that we're talking about member operators.

test/Analysis/invalid-pos-fix.cpp

6 ↗

(On Diff #188713)

Random ideas on organizing tests so that to avoid adding thousands of one-test files:

• Give these objects fancier names and/or add a no-crash so that it was clear that this test tests a crash for reporting a bug over a MemberExpr that corresponds to an operator()
• Or maybe wrap this into a namespace, so that it was easier to expand this file.
• We traditionally put such tests into test/Analysis/diagnostics, dunno why though.

etc.

I also recall @george.karpenkov arguing for making test directory structure mirror source directory structure, but that's definitely not a blocker for this patch :]

11–13 ↗

(On Diff #188713)

// expected-warning 3 {{Statement}}

[Szelethus]

In D58777#1422248, @NoQ wrote:

Very nice! Crashing diagnostic locations are a fairly common issue. That said, this still makes me wonder what sort of checker were you writing that triggered this originally :)

Unfortunately, nothing fancy, I just made another debug checker to emit a warning on each macro location to test macro expansions in the plist output. I did encounter a crash (this one), but since creduce can only work on a preprocessed file, I created this checker to be able to get a sensible test case.

[NoQ]

In D58777#1423809, @Szelethus wrote:

creduce can only work on a preprocessed file

clang -E -frewrite-includes to the rescue!

[Szelethus]

Fixes according to inline comments.

lib/StaticAnalyzer/Checkers/DebugCheckers.cpp

271

Also would you eventually want to expand this checker with non-Stmt callbacks?

No :)

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

679

I mean, I have absolutely no idea how this happened :') The best I can do is to document that it does happen from time to time and why this is the solution to that.

test/Analysis/invalid-pos-fix.cpp

6 ↗

(On Diff #188713)

I also recall @george.karpenkov arguing for making test directory structure mirror source directory structure, but that's definitely not a blocker for this patch :]

I always wondered why we are all bunched up in test/Analysis.

docs/analyzer/developer-docs/DebugChecks.rst

 
 The debug.Stats checker collects various information about the analysis of each
 function, such as how many blocks were reached and if the analyzer timed out.
 
 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.

include/clang/StaticAnalyzer/Checkers/Checkers.td

 def ExprInspectionChecker : Checker<"ExprInspection">,
   HelpText<"Check the analyzer's understanding of expressions">,
   Documentation<NotDocumented>;
 
 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"
 
 
 //===----------------------------------------------------------------------===//
 // Clone Detection
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CloneDetectionAlpha in {

lib/StaticAnalyzer/Checkers/DebugCheckers.cpp

 
 void ento::registerExplodedGraphViewer(CheckerManager &mgr) {
   mgr.registerChecker<ExplodedGraphViewer>();
 }
 
 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;
+}

lib/StaticAnalyzer/Core/PathDiagnostic.cpp

do {
           L = ADC->getDecl()->getEndLoc();
         break;
       }
 
       L = UseEnd ? Parent->getEndLoc() : Parent->getBeginLoc();
     } while (!L.isValid());
   }
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
 }
 
 static PathDiagnosticLocation
 getLocationForCaller(const StackFrameContext *SFC,
                      const LocationContext *CallerCtx,
                      const SourceManager &SM) {
   const CFGBlock &Block = *SFC->getCallSiteBlock();
PathDiagnosticLocation::createConditionalColonLoc(
                                             const ConditionalOperator *CO,
                                             const SourceManager &SM) {
   return PathDiagnosticLocation(CO->getColonLoc(), SM, SingleLocK);
 }
 
 PathDiagnosticLocation
 PathDiagnosticLocation::createMemberLoc(const MemberExpr *ME,
                                         const SourceManager &SM) {
+
+  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
 PathDiagnosticLocation::createBeginBrace(const CompoundStmt *CS,
                                          const SourceManager &SM) {
   SourceLocation L = CS->getLBracLoc();
   return PathDiagnosticLocation(L, SM, SingleLocK);
 }

test/Analysis/diagnostics/invalid-srcloc-fix.cpp

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

test/Analysis/plist-html-macros.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
 // (sanity check)
 
 // 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
 
 // This tests two things: that the two calls to null_deref below are coalesced
 // into a single bug by both the plist and HTML diagnostics, and that the plist
 // diagnostics have a reference to the HTML diagnostics. (It would be nice to
 // check more carefully that the two actually match, but that's hard to write
 // in a lit RUN line.)