Page MenuHomePhabricator

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

Authored by Szelethus on Feb 28 2019, 4:46 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Feb 28 2019, 4:46 AM
Szelethus updated this revision to Diff 188713.Feb 28 2019, 4:47 AM

Added context.

NoQ accepted this revision.Mar 7 2019, 3:54 PM
NoQ added a subscriber: george.karpenkov.

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

This revision is now accepted and ready to land.Mar 7 2019, 3:54 PM
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 added a comment.Mar 10 2019, 2:19 PM

creduce can only work on a preprocessed file

clang -E -frewrite-includes to the rescue!

Szelethus updated this revision to Diff 190643.Mar 14 2019, 9:01 AM
Szelethus marked 7 inline comments as done.

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.

This revision was automatically updated to reflect the committed changes.