NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, szepet.
Herald added a project: clang.
I'm slowly cleaning up `BugReporter` as a preparation for porting clang-tidy to
use it exclusively as discussed in
http://lists.llvm.org/pipermail/cfe-dev/2019-August/063092.html . Basically,
we'll need to make a `BugReport` and a `BugReporter` that both can be compiled
with `CLANG_ENABLE_STATIC_ANALYZER=OFF`: no "error nodes", no nothing. So we'll
most likely make a clear separation between a basic bug report(er) and a
path-sensitive bug report(er), having them inherit from common bug report(er)
classes.
While figuring out which field/method goes where, i already made a couple of
trivial commits (rC369319 <https://reviews.llvm.org/rC369319>, rC369320
<https://reviews.llvm.org/rC369320>). This one, however, is relatively
interesting.
The `BugTypes` field is basically unused, and it definitely doesn't need to be
implemented as an //immutable// set (if at all). But once i removed it i
started noticing a bunch of null dereferences in
`generateDiagnosticForConsumerMap()`:
3060 std::unique_ptr<DiagnosticForConsumerMapTy> Out =
3061 generatePathDiagnostics(consumers, bugReports);
3062
3063 if (Out->empty()) // <== crash: Out is null!
3064 return Out;
This was fun to debug because it's obvious by looking at
`PathSensitiveBugReporter::generatePathDiagnostics()` that it never returns
null:
2643 std::unique_ptr<DiagnosticForConsumerMapTy>
2644 PathSensitiveBugReporter::generatePathDiagnostics(...) {
...
2649 auto Out = llvm::make_unique<DiagnosticForConsumerMapTy>();
...
2658 return Out;
2659 }
The debugger was also sure that we're dealing with a path-sensitive bug
reporter here. The answer turned out to be related to this tiny discussion
<https://reviews.llvm.org/D63227?id=204353#inline-562789>. Or, rather, here's a
report that's worth a thousand words:
F9827911: report-f2e935.html <https://reviews.llvm.org/F9827911>
So i decided to avoid the destructor minefield entirely, even if it means
calling flush manually.
Also, ugh, why is CloneChecker subscribed to `check::EndOfTranslationUnit`
which is a path-sensitive callback? :/
Repository:
rC Clang
https://reviews.llvm.org/D66460
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -609,6 +609,7 @@
// After all decls handled, run checkers on the entire TranslationUnit.
checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
+ BR.FlushReports();
RecVisitorBR = nullptr;
}
@@ -766,6 +767,9 @@
if (SyntaxCheckTimer)
SyntaxCheckTimer->stopTimer();
}
+
+ BR.FlushReports();
+
if ((Mode & AM_Path) && checkerMgr->hasPathSensitiveCheckers()) {
RunPathSensitiveChecks(D, IMode, VisitedCallees);
if (IMode != ExprEngine::Inline_Minimal)
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2221,7 +2221,9 @@
}
BugReporter::~BugReporter() {
- FlushReports();
+ // Make sure reports are flushed.
+ assert(StrBugTypes.empty() &&
+ "Destroying BugReporter before diagnostics are emitted!");
// Free the bug reports we are tracking.
for (const auto I : EQClassesVector)
@@ -2229,9 +2231,6 @@
}
void BugReporter::FlushReports() {
- if (BugTypes.isEmpty())
- return;
-
// We need to flush reports in deterministic order to ensure the order
// of the reports is consistent between runs.
for (const auto EQ : EQClassesVector)
@@ -2242,9 +2241,6 @@
// FIXME: There are leaks from checkers that assume that the BugTypes they
// create will be destroyed by the BugReporter.
llvm::DeleteContainerSeconds(StrBugTypes);
-
- // Remove all references to the BugType objects.
- BugTypes = F.getEmptySet();
}
//===----------------------------------------------------------------------===//
@@ -2658,10 +2654,6 @@
return Out;
}
-void BugReporter::Register(const BugType *BT) {
- BugTypes = F.add(BugTypes, BT);
-}
-
void BugReporter::emitReport(std::unique_ptr<BugReport> R) {
if (const ExplodedNode *E = R->getErrorNode()) {
// An error node must either be a sink or have a tag, otherwise
@@ -2692,8 +2684,6 @@
R->Profile(ID);
// Lookup the equivance class. If there isn't one, create it.
- const BugType& BT = R->getBugType();
- Register(&BT);
void *InsertPos;
BugReportEquivClass* EQ = EQClasses.FindNodeOrInsertPos(ID, InsertPos);
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -406,11 +406,6 @@
enum Kind { BasicBRKind, PathSensitiveBRKind };
private:
- using BugTypesTy = llvm::ImmutableSet<BugType *>;
-
- BugTypesTy::Factory F;
- BugTypesTy BugTypes;
-
const Kind kind;
BugReporterData& D;
@@ -431,11 +426,10 @@
protected:
BugReporter(BugReporterData& d, Kind k)
- : BugTypes(F.getEmptySet()), kind(k), D(d) {}
+ : kind(k), D(d) {}
public:
- BugReporter(BugReporterData& d)
- : BugTypes(F.getEmptySet()), kind(BasicBRKind), D(d) {}
+ BugReporter(BugReporterData &d) : kind(BasicBRKind), D(d) {}
virtual ~BugReporter();
/// Generate and flush diagnostics for all bug reports.
@@ -451,11 +445,6 @@
return D.getPathDiagnosticConsumers();
}
- /// Iterator over the set of BugTypes tracked by the BugReporter.
- using iterator = BugTypesTy::iterator;
- iterator begin() { return BugTypes.begin(); }
- iterator end() { return BugTypes.end(); }
-
/// Iterator over the set of BugReports tracked by the BugReporter.
using EQClasses_iterator = llvm::FoldingSet<BugReportEquivClass>::iterator;
EQClasses_iterator EQClasses_begin() { return EQClasses.begin(); }
@@ -473,8 +462,6 @@
return {};
}
- void Register(const BugType *BT);
-
/// Add the given report to the set of reports tracked by BugReporter.
///
/// The reports are usually generated by the checkers. Further, they are
@@ -509,8 +496,6 @@
PathSensitiveBugReporter(BugReporterData& d, ExprEngine& eng)
: BugReporter(d, PathSensitiveBRKind), Eng(eng) {}
- ~PathSensitiveBugReporter() override = default;
-
/// getGraph - Get the exploded graph created by the analysis engine
/// for the analyzed method or function.
const ExplodedGraph &getGraph() const;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
