diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -125,6 +125,9 @@ /// int x; /// x = 42; Assignment, + /// The value got stored into the parameter region as the result + /// of a call. + CallArgument, /// The value got stored into the region as block capture. /// Block data is modeled as a separate region, thus whenever /// the analyzer sees a captured variable, its value is copied @@ -138,7 +141,7 @@ const ExplodedNode *StoreSite; /// The expression where the value comes from. /// NOTE: might be null. - Expr *SourceOfTheValue; + const Expr *SourceOfTheValue; /// Symbolic value that is being stored. SVal Value; /// Memory regions involved in the store operation. @@ -230,7 +233,8 @@ /// \param Opts Tracking options specifying how we got to it. /// /// NOTE: this method is designed for sub-trackers and visitors. - virtual PathDiagnosticPieceRef handle(StoreInfo SI, TrackingOptions Opts); + virtual PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC, + TrackingOptions Opts); /// Add custom expression handler with the highest priority. /// @@ -323,7 +327,8 @@ /// /// \return the produced note, null if the handler doesn't support this kind /// of stores. - virtual PathDiagnosticPieceRef handle(StoreInfo SI, TrackingOptions Opts) = 0; + virtual PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC, + TrackingOptions Opts) = 0; Tracker &getParentTracker() { return ParentTracker; } }; @@ -353,9 +358,7 @@ /// statement. Note that returning \c true does not actually imply /// that any visitors were added. bool trackExpressionValue(const ExplodedNode *N, const Expr *E, - PathSensitiveBugReport &R, - TrackingKind TKind = TrackingKind::Thorough, - bool EnableNullFPSuppression = true); + PathSensitiveBugReport &R, TrackingOptions Opts = {}); /// Track how the value got stored into the given region and where it came /// from. diff --git a/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp @@ -284,8 +284,9 @@ N); R->addRange(RS->getSourceRange()); - bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, - bugreporter::TrackingKind::Thorough, false); + bugreporter::trackExpressionValue( + N, RS->getRetValue(), *R, + {bugreporter::TrackingKind::Thorough, /*EnableNullFPSuppression=*/false}); C.emitReport(std::move(R)); } diff --git a/clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp @@ -148,7 +148,7 @@ *BT, "Index is out of bounds", N); R->addRange(IdxExpr->getSourceRange()); bugreporter::trackExpressionValue( - N, IdxExpr, *R, bugreporter::TrackingKind::Thorough, false); + N, IdxExpr, *R, {bugreporter::TrackingKind::Thorough, false}); C.emitReport(std::move(R)); return; } diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1232,12 +1232,7 @@ SVal V; bool Satisfied = false; - /// If the visitor is tracking the value directly responsible for the - /// bug, we are going to employ false positive suppression. - bool EnableNullFPSuppression; - - using TrackingKind = bugreporter::TrackingKind; - TrackingKind TKind; + TrackingOptions Options; const StackFrameContext *OriginSFC; public: @@ -1252,11 +1247,9 @@ /// this visitor can prevent that without polluting the bugpath too /// much. StoreSiteFinder(bugreporter::TrackerRef ParentTracker, KnownSVal V, - const MemRegion *R, bool InEnableNullFPSuppression, - TrackingKind TKind, + const MemRegion *R, TrackingOptions Options, const StackFrameContext *OriginSFC = nullptr) - : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), - EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind), + : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), Options(Options), OriginSFC(OriginSFC) { assert(R); } @@ -1273,8 +1266,8 @@ ID.AddPointer(&tag); ID.AddPointer(R); ID.Add(V); - ID.AddInteger(static_cast(TKind)); - ID.AddBoolean(EnableNullFPSuppression); + ID.AddInteger(static_cast(Options.Kind)); + ID.AddBoolean(Options.EnableNullFPSuppression); } /// Returns true if \p N represents the DeclStmt declaring and initializing @@ -1533,8 +1526,7 @@ if (!IsParam) InitE = InitE->IgnoreParenCasts(); - getParentTracker().track(InitE, StoreSite, - {TKind, EnableNullFPSuppression}); + getParentTracker().track(InitE, StoreSite, Options); } // Let's try to find the region where the value came from. @@ -1605,7 +1597,7 @@ } } - if (TKind == TrackingKind::Condition && + if (Options.Kind == TrackingKind::Condition && OriginSFC && !OriginSFC->isParentOf(StoreSite->getStackFrame())) return nullptr; @@ -1613,60 +1605,41 @@ SmallString<256> sbuf; llvm::raw_svector_ostream os(sbuf); + StoreInfo SI = {StoreInfo::Assignment, // default kind + StoreSite, + InitE, + V, + R, + OldRegion}; + if (Optional PS = StoreSite->getLocationAs()) { const Stmt *S = PS->getStmt(); - const char *action = nullptr; const auto *DS = dyn_cast(S); const auto *VR = dyn_cast(R); if (DS) { - action = R->canPrintPretty() ? "initialized to " : - "Initializing to "; + SI.StoreKind = StoreInfo::Initialization; } else if (isa(S)) { - action = R->canPrintPretty() ? "captured by block as " : - "Captured by block as "; + SI.StoreKind = StoreInfo::BlockCapture; if (VR) { // See if we can get the BlockVarRegion. ProgramStateRef State = StoreSite->getState(); SVal V = StoreSite->getSVal(S); if (const auto *BDR = - dyn_cast_or_null(V.getAsRegion())) { + dyn_cast_or_null(V.getAsRegion())) { if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) { if (auto KV = State->getSVal(OriginalR).getAs()) - getParentTracker().track( - *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC); + getParentTracker().track(*KV, OriginalR, Options, OriginSFC); } } } } - if (action) - showBRDiagnostics(action, os, R, V, OldRegion, DS); - - } else if (StoreSite->getLocation().getAs()) { - if (const auto *VR = dyn_cast(R)) - showBRParamDiagnostics(os, VR, V, OldRegion); + } else if (SI.StoreSite->getLocation().getAs() && + isa(SI.Dest)) { + SI.StoreKind = StoreInfo::CallArgument; } - if (os.str().empty()) - showBRDefaultDiagnostics(os, R, V, OldRegion); - - if (TKind == bugreporter::TrackingKind::Condition) - os << WillBeUsedForACondition; - - // Construct a new PathDiagnosticPiece. - ProgramPoint P = StoreSite->getLocation(); - PathDiagnosticLocation L; - if (P.getAs() && InitE) - L = PathDiagnosticLocation(InitE, BRC.getSourceManager(), - P.getLocationContext()); - - if (!L.isValid() || !L.asLocation().isValid()) - L = PathDiagnosticLocation::create(P, BRC.getSourceManager()); - - if (!L.isValid() || !L.asLocation().isValid()) - return nullptr; - - return std::make_shared(L, os.str()); + return getParentTracker().handle(SI, BRC, Options); } //===----------------------------------------------------------------------===// @@ -2060,6 +2033,63 @@ // Tracker implementation //===----------------------------------------------------------------------===// +class DefaultStoreHandler final : public StoreHandler { +public: + using StoreHandler::StoreHandler; + + PathDiagnosticPieceRef handle(StoreInfo SI, BugReporterContext &BRC, + TrackingOptions Opts) override { + // Okay, we've found the binding. Emit an appropriate message. + SmallString<256> Buffer; + llvm::raw_svector_ostream OS(Buffer); + const char *Action = nullptr; + + // TODO: get rid of it. + const DeclStmt *DS = nullptr; + + switch (SI.StoreKind) { + case StoreInfo::Initialization: + // We don't need to check here, all these conditions were + // checked by StoreSiteFinder, when it figured out that it is + // initialization. + DS = cast(SI.StoreSite->getLocationAs()->getStmt()); + Action = + SI.Dest->canPrintPretty() ? "initialized to " : "Initializing to "; + showBRDiagnostics(Action, OS, SI.Dest, SI.Value, SI.Origin, DS); + break; + case StoreInfo::BlockCapture: + Action = SI.Dest->canPrintPretty() ? "captured by block as " + : "Captured by block as "; + showBRDiagnostics(Action, OS, SI.Dest, SI.Value, SI.Origin, DS); + break; + case StoreInfo::CallArgument: + showBRParamDiagnostics(OS, cast(SI.Dest), SI.Value, SI.Origin); + break; + case StoreInfo::Assignment: + showBRDefaultDiagnostics(OS, SI.Dest, SI.Value, SI.Origin); + break; + } + + if (Opts.Kind == bugreporter::TrackingKind::Condition) + OS << WillBeUsedForACondition; + + // Construct a new PathDiagnosticPiece. + ProgramPoint P = SI.StoreSite->getLocation(); + PathDiagnosticLocation L; + if (P.getAs() && SI.SourceOfTheValue) + L = PathDiagnosticLocation(SI.SourceOfTheValue, BRC.getSourceManager(), + P.getLocationContext()); + + if (!L.isValid() || !L.asLocation().isValid()) + L = PathDiagnosticLocation::create(P, BRC.getSourceManager()); + + if (!L.isValid() || !L.asLocation().isValid()) + return nullptr; + + return std::make_shared(L, OS.str()); + } +}; + class DefaultExpressionHandler final : public ExpressionHandler { public: using ExpressionHandler::ExpressionHandler; @@ -2264,10 +2294,11 @@ }; Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) { + // Default expression handlers. addHighPriorityHandler(); addLowPriorityHandler(); - // TODO: split trackExpressionValue and FindLastStoreBRVisitor into handlers - // and add them here. + // Default store handlers. + addHighPriorityHandler(); } Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N, @@ -2294,17 +2325,17 @@ Tracker::Result Tracker::track(SVal V, const MemRegion *R, TrackingOptions Opts, const StackFrameContext *Origin) { if (auto KV = V.getAs()) { - Report.addVisitor( - this, *KV, R, Opts.EnableNullFPSuppression, Opts.Kind, Origin); + Report.addVisitor(this, *KV, R, Opts, Origin); return {true}; } return {}; } -PathDiagnosticPieceRef Tracker::handle(StoreInfo SI, TrackingOptions Opts) { +PathDiagnosticPieceRef Tracker::handle(StoreInfo SI, BugReporterContext &BRC, + TrackingOptions Opts) { // Iterate through the handlers in the order according to their priorities. for (StoreHandlerPtr &Handler : StoreHandlers) { - if (PathDiagnosticPieceRef Result = Handler->handle(SI, Opts)) + if (PathDiagnosticPieceRef Result = Handler->handle(SI, BRC, Opts)) // If the handler produced a non-null piece, return it. // There is no need in asking other handlers. return Result; @@ -2314,12 +2345,11 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, const Expr *E, - PathSensitiveBugReport &report, - bugreporter::TrackingKind TKind, - bool EnableNullFPSuppression) { + PathSensitiveBugReport &Report, + TrackingOptions Opts) { - return Tracker::create(report) - ->track(E, InputNode, {TKind, EnableNullFPSuppression}) + return Tracker::create(Report) + ->track(E, InputNode, Opts) .FoundSomethingToTrack; } @@ -2376,9 +2406,9 @@ // The receiver was nil, and hence the method was skipped. // Register a BugReporterVisitor to issue a message telling us how // the receiver was null. - bugreporter::trackExpressionValue( - N, Receiver, BR, bugreporter::TrackingKind::Thorough, - /*EnableNullFPSuppression*/ false); + bugreporter::trackExpressionValue(N, Receiver, BR, + {bugreporter::TrackingKind::Thorough, + /*EnableNullFPSuppression*/ false}); // Issue a message saying that the method was skipped. PathDiagnosticLocation L(Receiver, BRC.getSourceManager(), N->getLocationContext());