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 @@ -41,6 +41,8 @@ #include #include +#include + namespace clang { class AnalyzerOptions; @@ -129,11 +131,8 @@ /// Used for ensuring the visitors are only added once. llvm::FoldingSet CallbacksSet; + std::set CallbacksHashes; - /// Used for clients to tell if the report's configuration has changed - /// since the last time they checked. - unsigned ConfigurationChangeToken = 0; - /// When set, this flag disables all callstack pruning from a diagnostic /// path. This is useful for some reports that want maximum fidelty /// when reporting an issue. @@ -229,10 +228,6 @@ bool isInteresting(SVal V); bool isInteresting(const LocationContext *LC); - unsigned getConfigurationChangeToken() const { - return ConfigurationChangeToken; - } - /// Returns whether or not this report should be considered valid. /// /// Invalid reports are those that have been classified as likely false @@ -351,6 +346,7 @@ /// registerFindLastStore(), registerNilReceiverVisitor(), and /// registerVarDeclsLastStore(). void addVisitor(std::unique_ptr visitor); + void clearVisitors(); /// Iterators through the custom diagnostic visitors. visitor_iterator visitor_begin() { return Callbacks.begin(); } Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -39,12 +39,6 @@ class PathDiagnosticPiece; /// BugReporterVisitors are used to add custom diagnostics along a path. -/// -/// Custom visitors should subclass the BugReporterVisitorImpl class for a -/// default implementation of the clone() method. -/// (Warning: if you have a deep subclass of BugReporterVisitorImpl, the -/// default implementation of clone() will NOT do the right thing, and you -/// will have to provide your own implementation.) class BugReporterVisitor : public llvm::FoldingSetNode { public: BugReporterVisitor() = default; @@ -52,17 +46,6 @@ BugReporterVisitor(BugReporterVisitor &&) {} virtual ~BugReporterVisitor(); - /// Returns a copy of this BugReporter. - /// - /// Custom BugReporterVisitors should not override this method directly. - /// Instead, they should inherit from BugReporterVisitorImpl and provide - /// a protected or public copy constructor. - /// - /// (Warning: if you have a deep subclass of BugReporterVisitorImpl, the - /// default implementation of clone() will NOT do the right thing, and you - /// will have to provide your own implementation.) - virtual std::unique_ptr clone() const = 0; - /// Return a diagnostic piece which should be associated with the /// given node. /// @@ -89,23 +72,7 @@ BugReport &BR); }; -/// This class provides a convenience implementation for clone() using the -/// Curiously-Recurring Template Pattern. If you are implementing a custom -/// BugReporterVisitor, subclass BugReporterVisitorImpl and provide a public -/// or protected copy constructor. -/// -/// (Warning: if you have a deep subclass of BugReporterVisitorImpl, the -/// default implementation of clone() will NOT do the right thing, and you -/// will have to provide your own implementation.) -template -class BugReporterVisitorImpl : public BugReporterVisitor { - std::unique_ptr clone() const override { - return llvm::make_unique(*static_cast(this)); - } -}; - -class FindLastStoreBRVisitor final - : public BugReporterVisitorImpl { +class FindLastStoreBRVisitor final : public BugReporterVisitor { const MemRegion *R; SVal V; bool Satisfied = false; @@ -133,7 +100,7 @@ }; class TrackConstraintBRVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { DefinedSVal Constraint; bool Assumption; bool IsSatisfied = false; @@ -166,8 +133,7 @@ /// \class NilReceiverBRVisitor /// Prints path notes when a message is sent to a nil receiver. -class NilReceiverBRVisitor final - : public BugReporterVisitorImpl { +class NilReceiverBRVisitor final : public BugReporterVisitor { public: void Profile(llvm::FoldingSetNodeID &ID) const override { static int x = 0; @@ -185,8 +151,7 @@ }; /// Visitor that tries to report interesting diagnostics from conditions. -class ConditionBRVisitor final - : public BugReporterVisitorImpl { +class ConditionBRVisitor final : public BugReporterVisitor { // FIXME: constexpr initialization isn't supported by MSVC2013. static const char *const GenericTrueMessage; static const char *const GenericFalseMessage; @@ -249,7 +214,7 @@ /// /// Currently this suppresses reports based on locations of bugs. class LikelyFalsePositiveSuppressionBRVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { public: static void *getTag() { static int Tag = 0; @@ -278,7 +243,7 @@ /// As a result, BugReporter will not prune the path through the function even /// if the region's contents are not modified/accessed by the call. class UndefOrNullArgVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { /// The interesting memory region this visitor is tracking. const MemRegion *R; @@ -297,8 +262,7 @@ BugReport &BR) override; }; -class SuppressInlineDefensiveChecksVisitor final - : public BugReporterVisitorImpl { +class SuppressInlineDefensiveChecksVisitor final : public BugReporterVisitor { /// The symbolic value for which we are tracking constraints. /// This value is constrained to null in the end of path. DefinedSVal V; @@ -329,7 +293,7 @@ }; class CXXSelfAssignmentBRVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { bool Satisfied = false; public: @@ -345,7 +309,7 @@ /// The bug visitor prints a diagnostic message at the location where a given /// variable was tainted. -class TaintBugVisitor final : public BugReporterVisitorImpl { +class TaintBugVisitor final : public BugReporterVisitor { private: const SVal V; Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -859,7 +859,8 @@ filesmap_iterator executedLines_end() const { return ExecutedLines->end(); } PathDiagnosticLocation getLocation() const { - assert(Loc.isValid() && "No report location set yet!"); + // TODO??? + //assert(Loc.isValid() && "No report location set yet!"); return Loc; } Index: clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -39,7 +39,7 @@ : public Checker> { mutable std::unique_ptr BT; - class DeleteBugVisitor : public BugReporterVisitorImpl { + class DeleteBugVisitor : public BugReporterVisitor { public: DeleteBugVisitor() : Satisfied(false) {} void Profile(llvm::FoldingSetNodeID &ID) const override { Index: clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp @@ -39,7 +39,7 @@ } class DynamicTypeBugVisitor - : public BugReporterVisitorImpl { + : public BugReporterVisitor { public: DynamicTypeBugVisitor(const MemRegion *Reg) : Reg(Reg) {} Index: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -74,7 +74,7 @@ new BugType(this, "Generics", categories::CoreFoundationObjectiveC)); } - class GenericsBugVisitor : public BugReporterVisitorImpl { + class GenericsBugVisitor : public BugReporterVisitor { public: GenericsBugVisitor(SymbolRef S) : Sym(S) {} Index: clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp @@ -114,7 +114,7 @@ namespace { class NonLocalizedStringBRVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { const MemRegion *NonLocalizedString; bool Satisfied; Index: clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h +++ clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h @@ -78,7 +78,7 @@ /// Bug visitor class to find the node where the request region was previously /// used in order to include it into the BugReport path. - class RequestNodeVisitor : public BugReporterVisitorImpl { + class RequestNodeVisitor : public BugReporterVisitor { public: RequestNodeVisitor(const MemRegion *const MemoryRegion, const std::string &ErrText) Index: clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -121,7 +121,7 @@ /// BugReport path. For example, showing the allocation site of the leaked /// region. class SecKeychainBugVisitor - : public BugReporterVisitorImpl { + : public BugReporterVisitor { protected: // The allocated region symbol tracked by the main analysis. SymbolRef Sym; Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -430,7 +430,7 @@ /// BugReport path. For example, showing the allocation site of the leaked /// region. class MallocBugVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { protected: enum NotificationMode { Normal, Index: clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -61,7 +61,7 @@ private: enum MisuseKind {MK_FunCall, MK_Copy, MK_Move}; - class MovedBugVisitor : public BugReporterVisitorImpl { + class MovedBugVisitor : public BugReporterVisitor { public: MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -128,8 +128,7 @@ DefaultBool NeedTracking; private: - class NullabilityBugVisitor - : public BugReporterVisitorImpl { + class NullabilityBugVisitor : public BugReporterVisitor { public: NullabilityBugVisitor(const MemRegion *M) : Region(M) {} Index: clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -63,7 +63,7 @@ namespace { class SuperDeallocBRVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { SymbolRef ReceiverSymbol; bool Satisfied; Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -1789,7 +1789,7 @@ // Bug Reports. // //===---------===// - class CFRefReportVisitor : public BugReporterVisitorImpl { + class CFRefReportVisitor : public BugReporterVisitor { protected: SymbolRef Sym; const SummaryLogTy &SummaryLog; @@ -1825,14 +1825,6 @@ const ExplodedNode *N, BugReport &BR) override; - std::unique_ptr clone() const override { - // The curiously-recurring template pattern only works for one level of - // subclassing. Rather than make a new template base for - // CFRefReportVisitor, we simply override clone() to do the right thing. - // This could be trouble someday if BugReporterVisitorImpl is ever - // used for something else besides a convenient implementation of clone(). - return llvm::make_unique(*this); - } }; class CFRefReport : public BugReport { Index: clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp @@ -55,7 +55,7 @@ } }; -class DivisionBRVisitor : public BugReporterVisitorImpl { +class DivisionBRVisitor : public BugReporterVisitor { private: SymbolRef ZeroSymbol; const StackFrameContext *SFC; Index: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp @@ -69,7 +69,7 @@ bool IsCopy) const; void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const; - class ValistBugVisitor : public BugReporterVisitorImpl { + class ValistBugVisitor : public BugReporterVisitor { public: ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false) : Reg(Reg), IsLeak(IsLeak) {} Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -57,7 +57,7 @@ void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg, CheckerContext &C) const; - class VirtualBugVisitor : public BugReporterVisitorImpl { + class VirtualBugVisitor : public BugReporterVisitor { private: const MemRegion *ObjectRegion; bool Found; Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -1267,18 +1267,19 @@ /// Otherwise, more detailed diagnostics is emitted for block edges, explaining /// the transitions in words. static bool generatePathDiagnostics( + BugReport *R, PathDiagnostic &PD, PathDiagnosticBuilder &PDB, const ExplodedNode *N, LocationContextMap &LCM, - ArrayRef> visitors, PathDiagnosticConsumer::PathGenerationScheme ActiveScheme) { BugReport *report = PDB.getBugReport(); StackDiagVector CallStack; InterestingExprs IE; bool AddPathEdges = (ActiveScheme == PathDiagnosticConsumer::Extensive); bool GenerateDiagnostics = (ActiveScheme != PathDiagnosticConsumer::None); + const ExplodedNode *LastNode = N; - PathDiagnosticLocation PrevLoc = GenerateDiagnostics ? - PD.getLocation() : PathDiagnosticLocation(); + PathDiagnosticLocation PrevLoc = PD.getLocation(); + BugReport::VisitorList visitors; const ExplodedNode *NextNode = N->getFirstPred(); while (NextNode) { @@ -1292,6 +1293,13 @@ if (!NextNode) continue; + // Visitors may add new visitors to BugReport `R`. + // At each iteration, we move all visitors from `R` to `visitors`. + for (BugReport::visitor_iterator I = R->visitor_begin(), + E = R->visitor_end(); I != E; ++I) + visitors.push_back(std::move(*I)); + R->clearVisitors(); + // Add pieces from custom visitors. llvm::FoldingSet DeduplicationSet; for (auto &V : visitors) { @@ -1321,6 +1329,22 @@ CalleeLC); } + if (GenerateDiagnostics) { + std::unique_ptr LastPiece; + for (auto &V : visitors) { + if (std::unique_ptr Piece = V->getEndPath(PDB, LastNode, *R)) { + assert(!LastPiece && + "There can only be one final piece in a diagnostic."); + LastPiece = std::move(Piece); + } + } + + if (!LastPiece) + LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, LastNode, *R); + assert(LastPiece); + PD.setEndOfPath(std::move(LastPiece)); + } + if (!report->isValid()) return false; @@ -1977,14 +2001,17 @@ llvm::FoldingSetNodeID ID; visitor->Profile(ID); - void *InsertPos; - if (CallbacksSet.FindNodeOrInsertPos(ID, InsertPos)) + unsigned HashValue = ID.ComputeHash(); + auto P = CallbacksHashes.insert(HashValue); + if (!P.second) return; - CallbacksSet.InsertNode(visitor.get(), InsertPos); Callbacks.push_back(std::move(visitor)); - ++ConfigurationChangeToken; +} + +void BugReport::clearVisitors() { + Callbacks.clear(); } BugReport::~BugReport() { @@ -2030,9 +2057,7 @@ if (!sym) return; - // If the symbol wasn't already in our set, note a configuration change. - if (getInterestingSymbols().insert(sym).second) - ++ConfigurationChangeToken; + getInterestingSymbols().insert(sym); if (const auto *meta = dyn_cast(sym)) getInterestingRegions().insert(meta->getRegion()); @@ -2042,10 +2067,8 @@ if (!R) return; - // If the base region wasn't already in our set, note a configuration change. R = R->getBaseRegion(); - if (getInterestingRegions().insert(R).second) - ++ConfigurationChangeToken; + getInterestingRegions().insert(R); if (const auto *SR = dyn_cast(R)) getInterestingSymbols().insert(SR->getSymbol()); @@ -2533,59 +2556,9 @@ R->addVisitor(llvm::make_unique()); R->addVisitor(llvm::make_unique()); - BugReport::VisitorList visitors; - unsigned origReportConfigToken, finalReportConfigToken; LocationContextMap LCM; - // While generating diagnostics, it's possible the visitors will decide - // new symbols and regions are interesting, or add other visitors based on - // the information they find. If they do, we need to regenerate the path - // based on our new report configuration. - int jj=0; - do { // TODO: dump statistics on the MAX number of iterations of this loop. - jj++; - assert(jj<10); - // Get a clean copy of all the visitors. - for (BugReport::visitor_iterator I = R->visitor_begin(), - E = R->visitor_end(); I != E; ++I) - visitors.push_back((*I)->clone()); - - // Clear out the active path from any previous work. - PD.resetPath(); - origReportConfigToken = R->getConfigurationChangeToken(); - - // Generate the very last diagnostic piece - the piece is visible before - // the trace is expanded. - std::unique_ptr LastPiece; - for (BugReport::visitor_iterator I = visitors.begin(), E = visitors.end(); - I != E; ++I) { - if (std::unique_ptr Piece = - (*I)->getEndPath(PDB, N, *R)) { - assert(!LastPiece && - "There can only be one final piece in a diagnostic."); - LastPiece = std::move(Piece); - } - } - - if (ActiveScheme != PathDiagnosticConsumer::None) { - if (!LastPiece) - LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R); - assert(LastPiece); - PD.setEndOfPath(std::move(LastPiece)); - } - - // Make sure we get a clean location context map so we don't - // hold onto old mappings. - LCM.clear(); - - generatePathDiagnostics(PD, PDB, N, LCM, visitors, ActiveScheme); - - // Clean up the visitors we used. - visitors.clear(); - - // Did anything change while generating this path? - finalReportConfigToken = R->getConfigurationChangeToken(); - } while (finalReportConfigToken != origReportConfigToken); + generatePathDiagnostics(R, PD, PDB, N, LCM, ActiveScheme); if (!R->isValid()) continue; Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -227,7 +227,7 @@ /// but not written inside, and it has caused an undefined read or a null /// pointer dereference outside. class NoStoreFuncVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { const SubRegion *RegionOfInterest; static constexpr const char *DiagnosticsMsg = "Returning without writing to '"; @@ -519,7 +519,7 @@ }; class MacroNullReturnSuppressionVisitor final - : public BugReporterVisitorImpl { + : public BugReporterVisitor { const SubRegion *RegionOfInterest; public: @@ -601,7 +601,7 @@ /// /// This visitor is intended to be used when another visitor discovers that an /// interesting value comes from an inlined function call. -class ReturnVisitor : public BugReporterVisitorImpl { +class ReturnVisitor : public BugReporterVisitor { const StackFrameContext *StackFrame; enum { Initial,