From: Artem Dergachev Date: Wed, 11 Sep 2019 20:54:17 +0000 (+0000) Subject: [analyzer] NFC: Re-implement stack hints as a side map in BugReport. X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8535b8ecf2913b1a53571624da04890174381afe;p=platform%2Fupstream%2Fllvm.git [analyzer] NFC: Re-implement stack hints as a side map in BugReport. That's one of the few random entities in the PathDiagnostic interface that are specific to the Static Analyzer. By moving them out we could let everybody use path diagnostics without linking against Static Analyzer. Differential Revision: https://reviews.llvm.org/D67381 llvm-svn: 371658 --- diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h index 54aaa0b..6716ebe 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -70,6 +70,50 @@ class SValBuilder; using DiagnosticForConsumerMapTy = llvm::DenseMap>; +/// Interface for classes constructing Stack hints. +/// +/// If a PathDiagnosticEvent occurs in a different frame than the final +/// diagnostic the hints can be used to summarize the effect of the call. +class StackHintGenerator { +public: + virtual ~StackHintGenerator() = 0; + + /// Construct the Diagnostic message for the given ExplodedNode. + virtual std::string getMessage(const ExplodedNode *N) = 0; +}; + +/// Constructs a Stack hint for the given symbol. +/// +/// The class knows how to construct the stack hint message based on +/// traversing the CallExpr associated with the call and checking if the given +/// symbol is returned or is one of the arguments. +/// The hint can be customized by redefining 'getMessageForX()' methods. +class StackHintGeneratorForSymbol : public StackHintGenerator { +private: + SymbolRef Sym; + std::string Msg; + +public: + StackHintGeneratorForSymbol(SymbolRef S, StringRef M) : Sym(S), Msg(M) {} + ~StackHintGeneratorForSymbol() override = default; + + /// Search the call expression for the symbol Sym and dispatch the + /// 'getMessageForX()' methods to construct a specific message. + std::string getMessage(const ExplodedNode *N) override; + + /// Produces the message of the following form: + /// 'Msg via Nth parameter' + virtual std::string getMessageForArg(const Expr *ArgE, unsigned ArgIndex); + + virtual std::string getMessageForReturn(const CallExpr *CallExpr) { + return Msg; + } + + virtual std::string getMessageForSymbolNotFound() { + return Msg; + } +}; + /// This class provides an interface through which checkers can create /// individual bug reports. class BugReport { @@ -313,6 +357,14 @@ protected: const Stmt *getStmt() const; + /// If an event occurs in a different frame than the final diagnostic, + /// supply a message that will be used to construct an extra hint on the + /// returns from all the calls on the stack from this event to the final + /// diagnostic. + // FIXME: Allow shared_ptr keys in DenseMap? + std::map> + StackHints; + public: PathSensitiveBugReport(const BugType &bt, StringRef desc, const ExplodedNode *errorNode) @@ -455,6 +507,26 @@ public: bool addTrackedCondition(const ExplodedNode *Cond) { return TrackedConditions.insert(Cond).second; } + + void addCallStackHint(PathDiagnosticPieceRef Piece, + std::unique_ptr StackHint) { + StackHints[Piece] = std::move(StackHint); + } + + bool hasCallStackHint(PathDiagnosticPieceRef Piece) const { + return StackHints.count(Piece) > 0; + } + + /// Produce the hint for the given node. The node contains + /// information about the call for which the diagnostic can be generated. + std::string + getCallStackMessage(PathDiagnosticPieceRef Piece, + const ExplodedNode *N) const { + auto I = StackHints.find(Piece); + if (I != StackHints.end()) + return I->second->getMessage(N); + return ""; + } }; //===----------------------------------------------------------------------===// diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h index 289f6de..dd65af4 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -502,65 +502,13 @@ public: } }; -/// Interface for classes constructing Stack hints. -/// -/// If a PathDiagnosticEvent occurs in a different frame than the final -/// diagnostic the hints can be used to summarize the effect of the call. -class StackHintGenerator { -public: - virtual ~StackHintGenerator() = 0; - - /// Construct the Diagnostic message for the given ExplodedNode. - virtual std::string getMessage(const ExplodedNode *N) = 0; -}; - -/// Constructs a Stack hint for the given symbol. -/// -/// The class knows how to construct the stack hint message based on -/// traversing the CallExpr associated with the call and checking if the given -/// symbol is returned or is one of the arguments. -/// The hint can be customized by redefining 'getMessageForX()' methods. -class StackHintGeneratorForSymbol : public StackHintGenerator { -private: - SymbolRef Sym; - std::string Msg; - -public: - StackHintGeneratorForSymbol(SymbolRef S, StringRef M) : Sym(S), Msg(M) {} - ~StackHintGeneratorForSymbol() override = default; - - /// Search the call expression for the symbol Sym and dispatch the - /// 'getMessageForX()' methods to construct a specific message. - std::string getMessage(const ExplodedNode *N) override; - - /// Produces the message of the following form: - /// 'Msg via Nth parameter' - virtual std::string getMessageForArg(const Expr *ArgE, unsigned ArgIndex); - - virtual std::string getMessageForReturn(const CallExpr *CallExpr) { - return Msg; - } - - virtual std::string getMessageForSymbolNotFound() { - return Msg; - } -}; - class PathDiagnosticEventPiece : public PathDiagnosticSpotPiece { Optional IsPrunable; - /// If the event occurs in a different frame than the final diagnostic, - /// supply a message that will be used to construct an extra hint on the - /// returns from all the calls on the stack from this event to the final - /// diagnostic. - std::unique_ptr CallStackHint; - public: PathDiagnosticEventPiece(const PathDiagnosticLocation &pos, - StringRef s, bool addPosRange = true, - StackHintGenerator *stackHint = nullptr) - : PathDiagnosticSpotPiece(pos, s, Event, addPosRange), - CallStackHint(stackHint) {} + StringRef s, bool addPosRange = true) + : PathDiagnosticSpotPiece(pos, s, Event, addPosRange) {} ~PathDiagnosticEventPiece() override; /// Mark the diagnostic piece as being potentially prunable. This @@ -577,16 +525,6 @@ public: return IsPrunable.hasValue() ? IsPrunable.getValue() : false; } - bool hasCallStackHint() { return (bool)CallStackHint; } - - /// Produce the hint for the given node. The node contains - /// information about the call for which the diagnostic can be generated. - std::string getCallStackMessage(const ExplodedNode *N) { - if (CallStackHint) - return CallStackHint->getMessage(N); - return {}; - } - void dump() const override; static bool classof(const PathDiagnosticPiece *P) { diff --git a/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp index 89225d0..882df3e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -140,8 +140,7 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode( OS << "Conversion from derived to base happened here"; PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true, - nullptr); + return std::make_shared(Pos, OS.str(), true); } void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) { diff --git a/clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp index 7693314..d7a725c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp @@ -139,8 +139,7 @@ PathDiagnosticPieceRef DynamicTypeChecker::DynamicTypeBugVisitor::VisitNode( // Generate the extra diagnostic. PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true, - nullptr); + return std::make_shared(Pos, OS.str(), true); } static bool hasDefinition(const ObjCObjectPointerType *ObjPtr) { diff --git a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index f4ebc3d..ca2283d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -972,8 +972,7 @@ PathDiagnosticPieceRef DynamicTypePropagation::GenericsBugVisitor::VisitNode( // Generate the extra diagnostic. PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true, - nullptr); + return std::make_shared(Pos, OS.str(), true); } /// Register checkers. diff --git a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp index e8a2121..9cb77c0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp @@ -299,8 +299,7 @@ PathDiagnosticPieceRef InnerPointerChecker::InnerPointerBRVisitor::VisitNode( << "' obtained here"; PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true, - nullptr); + return std::make_shared(Pos, OS.str(), true); } void ento::registerInnerPointerChecker(CheckerManager &Mgr) { diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 6d06730..7763c53 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2953,15 +2953,15 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, // Find out if this is an interesting point and what is the kind. StringRef Msg; - StackHintGeneratorForSymbol *StackHint = nullptr; + std::unique_ptr StackHint = nullptr; SmallString<256> Buf; llvm::raw_svector_ostream OS(Buf); if (Mode == Normal) { if (isAllocated(RS, RSPrev, S)) { Msg = "Memory is allocated"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned allocated memory"); + StackHint = std::make_unique( + Sym, "Returned allocated memory"); } else if (isReleased(RS, RSPrev, S)) { const auto Family = RS->getAllocationFamily(); switch (Family) { @@ -2971,8 +2971,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, case AF_CXXNewArray: case AF_IfNameIndex: Msg = "Memory is released"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returning; memory was released"); + StackHint = std::make_unique( + Sym, "Returning; memory was released"); break; case AF_InnerBuffer: { const MemRegion *ObjRegion = @@ -2983,8 +2983,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) { OS << "deallocated by call to destructor"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returning; inner buffer was deallocated"); + StackHint = std::make_unique( + Sym, "Returning; inner buffer was deallocated"); } else { OS << "reallocated by call to '"; const Stmt *S = RS->getStmt(); @@ -2999,8 +2999,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, OS << (D ? D->getNameAsString() : "unknown"); } OS << "'"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returning; inner buffer was reallocated"); + StackHint = std::make_unique( + Sym, "Returning; inner buffer was reallocated"); } Msg = OS.str(); break; @@ -3040,12 +3040,12 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, } } else if (isRelinquished(RS, RSPrev, S)) { Msg = "Memory ownership is transferred"; - StackHint = new StackHintGeneratorForSymbol(Sym, ""); + StackHint = std::make_unique(Sym, ""); } else if (isReallocFailedCheck(RS, RSPrev, S)) { Mode = ReallocationFailed; Msg = "Reallocation failed"; - StackHint = new StackHintGeneratorForReallocationFailed(Sym, - "Reallocation failed"); + StackHint = std::make_unique( + Sym, "Reallocation failed"); if (SymbolRef sym = findFailedReallocSymbol(state, statePrev)) { // Is it possible to fail two reallocs WITHOUT testing in between? @@ -3064,16 +3064,15 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, if (!statePrev->get(FailedReallocSymbol)) { // We're at the reallocation point. Msg = "Attempt to reallocate memory"; - StackHint = new StackHintGeneratorForSymbol(Sym, - "Returned reallocated memory"); + StackHint = std::make_unique( + Sym, "Returned reallocated memory"); FailedReallocSymbol = nullptr; Mode = Normal; } } if (Msg.empty()) { - // Silence a memory leak warning by MallocChecker in MallocChecker.cpp :) - assert(!StackHint && "Memory leak!"); + assert(!StackHint); return nullptr; } @@ -3093,7 +3092,9 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, N->getLocationContext()); } - return std::make_shared(Pos, Msg, true, StackHint); + auto P = std::make_shared(Pos, Msg, true); + BR.addCallStackHint(P, std::move(StackHint)); + return P; } void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State, diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index f3a252c..4b68a4a 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -323,8 +323,7 @@ PathDiagnosticPieceRef NullabilityChecker::NullabilityBugVisitor::VisitNode( // Generate the extra diagnostic. PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, InfoText, true, - nullptr); + return std::make_shared(Pos, InfoText, true); } /// Returns true when the value stored at the given location has been diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index d3dacc7..c3b76b4 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -239,6 +239,8 @@ public: generate(const PathDiagnosticConsumer *PDC) const; private: + void updateStackPiecesWithMessage(PathDiagnosticPieceRef P, + const CallWithEntryStack &CallStack) const; void generatePathDiagnosticsForNode(PathDiagnosticConstruct &C, PathDiagnosticLocation &PrevLoc) const; @@ -270,6 +272,69 @@ private: } // namespace //===----------------------------------------------------------------------===// +// Base implementation of stack hint generators. +//===----------------------------------------------------------------------===// + +StackHintGenerator::~StackHintGenerator() = default; + +std::string StackHintGeneratorForSymbol::getMessage(const ExplodedNode *N){ + if (!N) + return getMessageForSymbolNotFound(); + + ProgramPoint P = N->getLocation(); + CallExitEnd CExit = P.castAs(); + + // FIXME: Use CallEvent to abstract this over all calls. + const Stmt *CallSite = CExit.getCalleeContext()->getCallSite(); + const auto *CE = dyn_cast_or_null(CallSite); + if (!CE) + return {}; + + // Check if one of the parameters are set to the interesting symbol. + unsigned ArgIndex = 0; + for (CallExpr::const_arg_iterator I = CE->arg_begin(), + E = CE->arg_end(); I != E; ++I, ++ArgIndex){ + SVal SV = N->getSVal(*I); + + // Check if the variable corresponding to the symbol is passed by value. + SymbolRef AS = SV.getAsLocSymbol(); + if (AS == Sym) { + return getMessageForArg(*I, ArgIndex); + } + + // Check if the parameter is a pointer to the symbol. + if (Optional Reg = SV.getAs()) { + // Do not attempt to dereference void*. + if ((*I)->getType()->isVoidPointerType()) + continue; + SVal PSV = N->getState()->getSVal(Reg->getRegion()); + SymbolRef AS = PSV.getAsLocSymbol(); + if (AS == Sym) { + return getMessageForArg(*I, ArgIndex); + } + } + } + + // Check if we are returning the interesting symbol. + SVal SV = N->getSVal(CE); + SymbolRef RetSym = SV.getAsLocSymbol(); + if (RetSym == Sym) { + return getMessageForReturn(CE); + } + + return getMessageForSymbolNotFound(); +} + +std::string StackHintGeneratorForSymbol::getMessageForArg(const Expr *ArgE, + unsigned ArgIndex) { + // Printed parameters start at 1, not 0. + ++ArgIndex; + + return (llvm::Twine(Msg) + " via " + std::to_string(ArgIndex) + + llvm::getOrdinalSuffix(ArgIndex) + " parameter").str(); +} + +//===----------------------------------------------------------------------===// // Helper routines for walking the ExplodedGraph and fetching statements. //===----------------------------------------------------------------------===// @@ -661,7 +726,7 @@ getEnclosingStmtLocation(const Stmt *S, const LocationContext *LC, //===----------------------------------------------------------------------===// /// If the piece contains a special message, add it to all the call pieces on -/// the active stack. For exampler, my_malloc allocated memory, so MallocChecker +/// the active stack. For example, my_malloc allocated memory, so MallocChecker /// will construct an event at the call to malloc(), and add a stack hint that /// an allocated memory was returned. We'll use this hint to construct a message /// when returning from the call to my_malloc @@ -670,22 +735,20 @@ getEnclosingStmtLocation(const Stmt *S, const LocationContext *LC, /// void fishy() { /// void *ptr = my_malloc(); // returned allocated memory /// } // leak -static void updateStackPiecesWithMessage(PathDiagnosticPiece &P, - const CallWithEntryStack &CallStack) { - if (auto *ep = dyn_cast(&P)) { - if (ep->hasCallStackHint()) - for (const auto &I : CallStack) { - PathDiagnosticCallPiece *CP = I.first; - const ExplodedNode *N = I.second; - std::string stackMsg = ep->getCallStackMessage(N); - - // The last message on the path to final bug is the most important - // one. Since we traverse the path backwards, do not add the message - // if one has been previously added. - if (!CP->hasCallStackMessage()) - CP->setCallStackMessage(stackMsg); - } - } +void PathDiagnosticBuilder::updateStackPiecesWithMessage( + PathDiagnosticPieceRef P, const CallWithEntryStack &CallStack) const { + if (R->hasCallStackHint(P)) + for (const auto &I : CallStack) { + PathDiagnosticCallPiece *CP = I.first; + const ExplodedNode *N = I.second; + std::string stackMsg = R->getCallStackMessage(P, N); + + // The last message on the path to final bug is the most important + // one. Since we traverse the path backwards, do not add the message + // if one has been previously added. + if (!CP->hasCallStackMessage()) + CP->setCallStackMessage(stackMsg); + } } static void CompactMacroExpandedPieces(PathPieces &path, @@ -1990,7 +2053,7 @@ PathDiagnosticBuilder::generate(const PathDiagnosticConsumer *PDC) const { if (PDC->shouldAddPathEdges()) addEdgeToPath(Construct.getActivePath(), PrevLoc, Note->getLocation()); - updateStackPiecesWithMessage(*Note, Construct.CallStack); + updateStackPiecesWithMessage(Note, Construct.CallStack); Construct.getActivePath().push_front(Note); } } diff --git a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index f049a1c..94d1c09 100644 --- a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -31,7 +31,6 @@ #include "clang/Basic/SourceManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/FoldingSet.h" #include "llvm/ADT/None.h" @@ -1303,70 +1302,6 @@ void PathDiagnostic::FullProfile(llvm::FoldingSetNodeID &ID) const { ID.AddString(*I); } -StackHintGenerator::~StackHintGenerator() = default; - -std::string StackHintGeneratorForSymbol::getMessage(const ExplodedNode *N){ - if (!N) - return getMessageForSymbolNotFound(); - - ProgramPoint P = N->getLocation(); - CallExitEnd CExit = P.castAs(); - - // FIXME: Use CallEvent to abstract this over all calls. - const Stmt *CallSite = CExit.getCalleeContext()->getCallSite(); - const auto *CE = dyn_cast_or_null(CallSite); - if (!CE) - return {}; - - // Check if one of the parameters are set to the interesting symbol. - unsigned ArgIndex = 0; - for (CallExpr::const_arg_iterator I = CE->arg_begin(), - E = CE->arg_end(); I != E; ++I, ++ArgIndex){ - SVal SV = N->getSVal(*I); - - // Check if the variable corresponding to the symbol is passed by value. - SymbolRef AS = SV.getAsLocSymbol(); - if (AS == Sym) { - return getMessageForArg(*I, ArgIndex); - } - - // Check if the parameter is a pointer to the symbol. - if (Optional Reg = SV.getAs()) { - // Do not attempt to dereference void*. - if ((*I)->getType()->isVoidPointerType()) - continue; - SVal PSV = N->getState()->getSVal(Reg->getRegion()); - SymbolRef AS = PSV.getAsLocSymbol(); - if (AS == Sym) { - return getMessageForArg(*I, ArgIndex); - } - } - } - - // Check if we are returning the interesting symbol. - SVal SV = N->getSVal(CE); - SymbolRef RetSym = SV.getAsLocSymbol(); - if (RetSym == Sym) { - return getMessageForReturn(CE); - } - - return getMessageForSymbolNotFound(); -} - -std::string StackHintGeneratorForSymbol::getMessageForArg(const Expr *ArgE, - unsigned ArgIndex) { - // Printed parameters start at 1, not 0. - ++ArgIndex; - - SmallString<200> buf; - llvm::raw_svector_ostream os(buf); - - os << Msg << " via " << ArgIndex << llvm::getOrdinalSuffix(ArgIndex) - << " parameter"; - - return os.str(); -} - LLVM_DUMP_METHOD void PathPieces::dump() const { unsigned index = 0; for (PathPieces::const_iterator I = begin(), E = end(); I != E; ++I) {