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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a subscriber: george.karpenkov.
NoQ added a comment.
This revision is now accepted and ready to land.

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 :)



================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:271
+//===----------------------------------------------------------------------===//
+// Emits a report for each and every Stmt.
+//===----------------------------------------------------------------------===//
----------------
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 `Stmt`s we more-or-less-intentionally only invoke `checkPreStmt` but not `checkPostStmt`.

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


================
Comment at: lib/StaticAnalyzer/Checkers/DebugCheckers.cpp:277
+class ReportStmts : public Checker<check::PreStmt<Stmt>> {
+  std::unique_ptr<BuiltinBug> BT_stmtLoc;
+
----------------
My current favorite approach is:
```lang=c++
BuiltinBug BT_stmtLoc{this, "Statement"};
```

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


================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:574-575
 
+  // FIXME: Ironically, this assert actually fails in some cases.
+  //assert(L.isValid());
   return L;
----------------
A normal day in Clang :)


================
Comment at: lib/StaticAnalyzer/Core/PathDiagnostic.cpp:679
+
+  if (ME->getMemberLoc().isValid())
+    return PathDiagnosticLocation(ME->getMemberLoc(), SM, SingleLocK);
----------------
I think it's worth commenting why exactly this may fail. It's fairly hard to guess that we're talking about member operators.


================
Comment at: test/Analysis/invalid-pos-fix.cpp:6
+
+struct h {
+  operator int();
----------------
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 :]


================
Comment at: test/Analysis/invalid-pos-fix.cpp:11-13
+  return h(); // expected-warning{{Statement}}
+              // expected-warning at -1{{Statement}}
+              // expected-warning at -2{{Statement}}
----------------
`// expected-warning 3 {{Statement}}`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58777/new/

https://reviews.llvm.org/D58777