Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h =================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h +++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h @@ -70,6 +70,50 @@ 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 @@ 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 @@ 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 ""; + } }; //===----------------------------------------------------------------------===// Index: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h =================================================================== --- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h +++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h @@ -502,65 +502,13 @@ } }; -/// 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 @@ 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) { Index: cfe/trunk/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -140,8 +140,7 @@ 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) { Index: cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp @@ -139,8 +139,7 @@ // 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) { Index: cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -972,8 +972,7 @@ // 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. Index: cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp @@ -299,8 +299,7 @@ << "' 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) { Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2953,15 +2953,15 @@ // 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 @@ 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 @@ 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 @@ 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 @@ } } 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 @@ 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 @@ 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, Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -323,8 +323,7 @@ // 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 Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp +++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -239,6 +239,8 @@ 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 @@ } // 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 @@ //===----------------------------------------------------------------------===// /// 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 @@ /// 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 @@ if (PDC->shouldAddPathEdges()) addEdgeToPath(Construct.getActivePath(), PrevLoc, Note->getLocation()); - updateStackPiecesWithMessage(*Note, Construct.CallStack); + updateStackPiecesWithMessage(Note, Construct.CallStack); Construct.getActivePath().push_front(Note); } } Index: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ cfe/trunk/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 @@ 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) {