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 @@ -352,6 +347,9 @@ /// registerVarDeclsLastStore(). void addVisitor(std::unique_ptr visitor); + /// Remove all visitors attached to this bug report. + void clearVisitors(); + /// Iterators through the custom diagnostic visitors. visitor_iterator visitor_begin() { return Callbacks.begin(); } visitor_iterator visitor_end() { return Callbacks.end(); } 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; @@ -132,8 +99,7 @@ BugReport &BR) override; }; -class TrackConstraintBRVisitor final - : public BugReporterVisitorImpl { +class TrackConstraintBRVisitor final : public BugReporterVisitor { DefinedSVal Constraint; bool Assumption; bool IsSatisfied = false; @@ -166,8 +132,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 +150,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 +213,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 +242,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 +261,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; @@ -328,8 +291,7 @@ BugReport &BR) override; }; -class CXXSelfAssignmentBRVisitor final - : public BugReporterVisitorImpl { +class CXXSelfAssignmentBRVisitor final : public BugReporterVisitor { bool Satisfied = false; public: @@ -345,7 +307,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/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 @@ -38,8 +38,7 @@ new BugType(this, "Dynamic and static type mismatch", "Type Error")); } - class DynamicTypeBugVisitor - : public BugReporterVisitorImpl { + class DynamicTypeBugVisitor : 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 @@ -113,8 +113,7 @@ } namespace { -class NonLocalizedStringBRVisitor final - : public BugReporterVisitorImpl { +class NonLocalizedStringBRVisitor final : 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 @@ -120,8 +120,7 @@ /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class SecKeychainBugVisitor - : public BugReporterVisitorImpl { + class SecKeychainBugVisitor : 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 @@ -429,8 +429,7 @@ /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class MallocBugVisitor final - : public BugReporterVisitorImpl { + class MallocBugVisitor final : 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 @@ -62,9 +62,7 @@ REGISTER_SET_WITH_PROGRAMSTATE(CalledSuperDealloc, SymbolRef) namespace { -class SuperDeallocBRVisitor final - : public BugReporterVisitorImpl { - +class SuperDeallocBRVisitor final : 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 @@ -1788,8 +1788,7 @@ //===---------===// // Bug Reports. // //===---------===// - - class CFRefReportVisitor : public BugReporterVisitorImpl { + class CFRefReportVisitor : public BugReporterVisitor { protected: SymbolRef Sym; const SummaryLogTy &SummaryLog; @@ -1824,15 +1823,6 @@ std::unique_ptr getEndPath(BugReporterContext &BRC, 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 @@ -1041,7 +1041,6 @@ // call exit before this point. This means that the path // terminated within the call itself. if (auto CE = P.getAs()) { - if (AddPathEdges) { // Add an edge to the start of the function. const StackFrameContext *CalleeLC = CE->getCalleeContext(); @@ -1267,9 +1266,9 @@ /// 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; @@ -1277,11 +1276,20 @@ bool AddPathEdges = (ActiveScheme == PathDiagnosticConsumer::Extensive); bool GenerateDiagnostics = (ActiveScheme != PathDiagnosticConsumer::None); - PathDiagnosticLocation PrevLoc = GenerateDiagnostics ? - PD.getLocation() : PathDiagnosticLocation(); + PathDiagnosticLocation PrevLoc = PD.getLocation(); + BugReport::VisitorList visitors; const ExplodedNode *NextNode = N->getFirstPred(); while (NextNode) { + + // 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(); + N = NextNode; NextNode = N->getFirstPred(); @@ -1977,14 +1985,18 @@ llvm::FoldingSetNodeID ID; visitor->Profile(ID); - void *InsertPos; - if (CallbacksSet.FindNodeOrInsertPos(ID, InsertPos)) + unsigned HashValue = ID.ComputeHash(); + // TODO: hashing is not perfect, store the entire ID instead. + 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 +2042,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 +2052,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,32 +2541,12 @@ 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. + // Only the initially added visitors can set the last diagnostic piece. + if (ActiveScheme != PathDiagnosticConsumer::None) { std::unique_ptr LastPiece; - for (BugReport::visitor_iterator I = visitors.begin(), E = visitors.end(); - I != E; ++I) { + for (BugReport::visitor_iterator I = R->visitor_begin(), + E = R->visitor_end(); + I != E; ++I) { if (std::unique_ptr Piece = (*I)->getEndPath(PDB, N, *R)) { assert(!LastPiece && @@ -2567,25 +2555,14 @@ } } - 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(); + if (!LastPiece) + LastPiece = BugReporterVisitor::getDefaultEndPath(PDB, N, *R); + assert(LastPiece); + PD.setEndOfPath(std::move(LastPiece)); + } - // Did anything change while generating this path? - finalReportConfigToken = R->getConfigurationChangeToken(); - } while (finalReportConfigToken != origReportConfigToken); + LocationContextMap LCM; + generatePathDiagnostics(R, PD, PDB, N, LCM, ActiveScheme); if (!R->isValid()) continue; @@ -3094,7 +3071,7 @@ if (const Stmt *SLoc = getLocStmt(getLocation())) SLoc->dump(); - else if (const auto *ND = dyn_cast(getCallee())) + else if (const auto *ND = dyn_cast_or_null(getCallee())) llvm::errs() << *ND << "\n"; else getLocation().dump(); Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -226,8 +226,7 @@ /// for which the region of interest \p RegionOfInterest was passed into, /// but not written inside, and it has caused an undefined read or a null /// pointer dereference outside. -class NoStoreFuncVisitor final - : public BugReporterVisitorImpl { +class NoStoreFuncVisitor final : public BugReporterVisitor { const SubRegion *RegionOfInterest; static constexpr const char *DiagnosticsMsg = "Returning without writing to '"; @@ -518,8 +517,7 @@ } }; -class MacroNullReturnSuppressionVisitor final - : public BugReporterVisitorImpl { +class MacroNullReturnSuppressionVisitor final : public BugReporterVisitor { const SubRegion *RegionOfInterest; public: @@ -601,7 +599,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,