From 4962816e7242b9cec7a1a1157e4efaac75a6120a Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Thu, 14 Mar 2019 16:10:29 +0000 Subject: [PATCH] [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 llvm-svn: 356161 --- clang/docs/analyzer/developer-docs/DebugChecks.rst | 7 +++++ .../clang/StaticAnalyzer/Checkers/Checkers.td | 4 +++ .../lib/StaticAnalyzer/Checkers/DebugCheckers.cpp | 31 ++++++++++++++++++++++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp | 12 ++++++++- .../Analysis/diagnostics/invalid-srcloc-fix.cpp | 12 +++++++++ clang/test/Analysis/plist-html-macros.c | 5 +++- 6 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 clang/test/Analysis/diagnostics/invalid-srcloc-fix.cpp diff --git a/clang/docs/analyzer/developer-docs/DebugChecks.rst b/clang/docs/analyzer/developer-docs/DebugChecks.rst index bb2f37f..56ce015 100644 --- a/clang/docs/analyzer/developer-docs/DebugChecks.rst +++ b/clang/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/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index 7c8a3c7..af5edae 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1019,6 +1019,10 @@ def ExplodedGraphViewer : Checker<"ViewExplodedGraph">, HelpText<"View Exploded Graphs using GraphViz">, Documentation; +def ReportStmts : Checker<"ReportStmts">, + HelpText<"Emits a warning for every statement.">, + Documentation; + } // end "debug" diff --git a/clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp b/clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp index 50715f5..63215e6 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp +++ b/clang/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> { + 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(BT_stmtLoc, "Statement", Node); + + C.emitReport(std::move(Report)); + } +}; + +} // end of anonymous namespace + +void ento::registerReportStmts(CheckerManager &mgr) { + mgr.registerChecker(); +} + +bool ento::shouldRegisterReportStmts(const LangOptions &LO) { + return true; +} diff --git a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index 13e3bcf..cc1e7e1 100644 --- a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/clang/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/clang/test/Analysis/diagnostics/invalid-srcloc-fix.cpp b/clang/test/Analysis/diagnostics/invalid-srcloc-fix.cpp new file mode 100644 index 0000000..0cef5e3 --- /dev/null +++ b/clang/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/clang/test/Analysis/plist-html-macros.c b/clang/test/Analysis/plist-html-macros.c index c25346d..0ac79be 100644 --- a/clang/test/Analysis/plist-html-macros.c +++ b/clang/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 -- 2.7.4