[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.