[clang] 0cc3100 - [analyzer] Introduce a new interface for tracking
Valeriy Savchenko via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 11 02:51:50 PDT 2021
Author: Valeriy Savchenko
Date: 2021-06-11T12:49:03+03:00
New Revision: 0cc3100bf8d126ce080c0075cf25784b45e5f990
URL: https://github.com/llvm/llvm-project/commit/0cc3100bf8d126ce080c0075cf25784b45e5f990
DIFF: https://github.com/llvm/llvm-project/commit/0cc3100bf8d126ce080c0075cf25784b45e5f990.diff
LOG: [analyzer] Introduce a new interface for tracking
Tracking values through expressions and the stores is fundamental
for producing clear diagnostics. However, the main components
participating in this process, namely `trackExpressionValue` and
`FindLastStoreBRVisitor`, became pretty bloated. They have an
interesting dynamic between them (and some other visitors) that
one might call a "chain reaction". `trackExpressionValue` adds
`FindLastStoreBRVisitor`, and the latter calls `trackExpressionValue`.
Because of this design, individual checkers couldn't affect what's
going to happen somewhere in the middle of that chain. Whether they
want to produce a more informative note or keep the overall tracking
going by utilizing some of the domain expertise. This all lead to two
biggest problems that I see:
* Some checkers don't use it
This should probably never be the case for path-sensitive checks.
* Some checkers incorporated their logic directly into those
components
This doesn't make the maintenance easier, breaks multiple
architecture principles, and makes the code harder to read adn
understand, thus, increasing the probability of the first case.
This commit introduces a prototype for a new interface that will be
responsible for tracking. My main idea here was to make operations
that I want have as a checker developer easy to implement and hook
directly into the tracking process.
Differential Revision: https://reviews.llvm.org/D103605
Added:
Modified:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
index 2975d50de3334..5c4345a32c8d3 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -21,7 +21,9 @@
#include "llvm/ADT/FoldingSet.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
+#include <list>
#include <memory>
+#include <utility>
namespace clang {
@@ -99,6 +101,223 @@ enum class TrackingKind {
Condition
};
+/// Defines a set of options altering tracking behavior.
+struct TrackingOptions {
+ /// Specifies the kind of tracking.
+ TrackingKind Kind = TrackingKind::Thorough;
+ /// Specifies whether we should employ false positive suppression
+ /// (inlined defensive checks, returned null).
+ bool EnableNullFPSuppression = true;
+};
+
+/// Describes an event when the value got stored into a memory region.
+///
+/// As opposed to checker checkBind API, it reacts also to binds
+/// generated by the checker as well. It can be useful when the binding
+/// happened as a result of evalCall, for example.
+struct StoreInfo {
+ enum Kind {
+ /// The value got stored into the region during initialization:
+ /// int x = 42;
+ Initialization,
+ /// The value got stored into the region during assignment:
+ /// int x;
+ /// x = 42;
+ Assignment,
+ /// 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
+ /// into a special block region.
+ BlockCapture
+ };
+
+ /// The type of store operation.
+ Kind StoreKind;
+ /// The node where the store happened.
+ const ExplodedNode *StoreSite;
+ /// The expression where the value comes from.
+ /// NOTE: might be null.
+ Expr *SourceOfTheValue;
+ /// Symbolic value that is being stored.
+ SVal Value;
+ /// Memory regions involved in the store operation.
+ /// Dest <- Origin
+ /// NOTE: Origin might be null, when the stored value doesn't come
+ /// from another region.
+ const MemRegion *Dest, *Origin;
+};
+
+class ExpressionHandler;
+class StoreHandler;
+
+/// A generalized component for tracking expressions, values, and stores.
+///
+/// Tracker aimes at providing a sensible set of default behaviors that can be
+/// used by any checker, while providing mechanisms to hook into any part of the
+/// tracking process and insert checker-specific logic.
+class Tracker {
+private:
+ using ExpressionHandlerPtr = std::unique_ptr<ExpressionHandler>;
+ using StoreHandlerPtr = std::unique_ptr<StoreHandler>;
+
+ PathSensitiveBugReport &Report;
+ std::list<ExpressionHandlerPtr> ExpressionHandlers;
+ std::list<StoreHandlerPtr> StoreHandlers;
+
+public:
+ /// \param Report The bug report to which visitors should be attached.
+ Tracker(PathSensitiveBugReport &Report);
+ virtual ~Tracker() = default;
+
+ PathSensitiveBugReport &getReport() { return Report; }
+
+ /// Describes a tracking result with the most basic information of what was
+ /// actually done (or not done).
+ struct Result {
+ /// Usually it means that the tracker added visitors.
+ bool FoundSomethingToTrack = false;
+ /// Signifies that the tracking was interrupted at some point.
+ /// Usually this information is important only for sub-trackers.
+ bool WasInterrupted = false;
+
+ /// Combines the current result with the given result.
+ void combineWith(const Result &Other) {
+ // If we found something in one of the cases, we can
+ // say we found something overall.
+ FoundSomethingToTrack |= Other.FoundSomethingToTrack;
+ // The same goes to the interruption.
+ WasInterrupted |= Other.WasInterrupted;
+ }
+ };
+
+ /// Track expression value back to its point of origin.
+ ///
+ /// \param E The expression value which we are tracking
+ /// \param N A node "downstream" from the evaluation of the statement.
+ /// \param Opts Tracking options specifying how we want to track the value.
+ virtual Result track(const Expr *E, const ExplodedNode *N,
+ TrackingOptions Opts = {});
+
+ /// Track how the value got stored into the given region and where it came
+ /// from.
+ ///
+ /// \param V We're searching for the store where \c R received this value.
+ /// \param R The region we're tracking.
+ /// \param Opts Tracking options specifying how we want to track the value.
+ /// \param Origin Only adds notes when the last store happened in a
+ ///
diff erent stackframe to this one. Disregarded if the tracking kind
+ /// is thorough.
+ /// This is useful, because for non-tracked regions, notes about
+ /// changes to its value in a nested stackframe could be pruned, and
+ /// this visitor can prevent that without polluting the bugpath too
+ /// much.
+ virtual Result track(SVal V, const MemRegion *R, TrackingOptions Opts = {},
+ const StackFrameContext *Origin = nullptr);
+
+ /// Handle the store operation and produce the note.
+ ///
+ /// \param SI The information fully describing the store.
+ /// \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);
+
+ /// Add custom expression handler with the highest priority.
+ ///
+ /// It means that it will be asked for handling first, and can prevent
+ /// other handlers from running if decides to interrupt.
+ void addHighPriorityHandler(ExpressionHandlerPtr SH) {
+ ExpressionHandlers.push_front(std::move(SH));
+ }
+
+ /// Add custom expression handler with the lowest priority.
+ ///
+ /// It means that it will be asked for handling last, and other handlers can
+ /// prevent it from running if any of them decides to interrupt.
+ void addLowPriorityHandler(ExpressionHandlerPtr SH) {
+ ExpressionHandlers.push_back(std::move(SH));
+ }
+
+ /// Add custom store handler with the highest priority.
+ ///
+ /// It means that it will be asked for handling first, and will prevent
+ /// other handlers from running if it produces non-null note.
+ void addHighPriorityHandler(StoreHandlerPtr SH) {
+ StoreHandlers.push_front(std::move(SH));
+ }
+
+ /// Add custom store handler with the lowest priority.
+ ///
+ /// It means that it will be asked for handling last, only
+ /// if all other handlers failed to produce the note.
+ void addLowPriorityHandler(StoreHandlerPtr SH) {
+ StoreHandlers.push_back(std::move(SH));
+ }
+
+ /// Add custom expression/store handler with the highest priority
+ ///
+ /// See other overloads for explanation.
+ template <class HandlerType, class... Args>
+ void addHighPriorityHandler(Args &&... ConstructorArgs) {
+ addHighPriorityHandler(std::make_unique<HandlerType>(
+ *this, std::forward<Args>(ConstructorArgs)...));
+ }
+
+ /// Add custom expression/store handler with the lowest priority
+ ///
+ /// See other overloads for explanation.
+ template <class HandlerType, class... Args>
+ void addLowPriorityHandler(Args &&... ConstructorArgs) {
+ addLowPriorityHandler(std::make_unique<HandlerType>(
+ *this, std::forward<Args>(ConstructorArgs)...));
+ }
+};
+
+/// Handles expressions during the tracking.
+class ExpressionHandler {
+private:
+ Tracker &ParentTracker;
+
+public:
+ ExpressionHandler(Tracker &ParentTracker) : ParentTracker(ParentTracker) {}
+ virtual ~ExpressionHandler() {}
+
+ /// Handle the given expression from the given node.
+ ///
+ /// \param E The expression value which we are tracking
+ /// \param Original A node "downstream" where the tracking started.
+ /// \param ExprNode A node where the evaluation of \c E actually happens.
+ /// \param Opts Tracking options specifying how we are tracking the value.
+ virtual Tracker::Result handle(const Expr *E, const ExplodedNode *Original,
+ const ExplodedNode *ExprNode,
+ TrackingOptions Opts) = 0;
+
+ /// \Return the tracker that initiated the process.
+ Tracker &getParentTracker() { return ParentTracker; }
+};
+
+/// Handles stores during the tracking.
+class StoreHandler {
+private:
+ Tracker &ParentTracker;
+
+public:
+ StoreHandler(Tracker &ParentTracker) : ParentTracker(ParentTracker) {}
+ virtual ~StoreHandler() {}
+
+ /// Handle the given store and produce the node.
+ ///
+ /// \param E The expression value which we are tracking
+ /// \param N A node where the evaluation of \c E actually happens.
+ /// \param Opts Tracking options specifying how we are tracking the value.
+ ///
+ /// \return the produced note, null if the handler doesn't support this kind
+ /// of stores.
+ virtual PathDiagnosticPieceRef handle(StoreInfo SI, TrackingOptions Opts) = 0;
+
+ Tracker &getParentTracker() { return ParentTracker; }
+};
+
/// Attempts to add visitors to track expression value back to its point of
/// origin.
///
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index bea1dfda975b2..9359a5fe80f1c 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -65,6 +65,7 @@
using namespace clang;
using namespace ento;
+using namespace bugreporter;
//===----------------------------------------------------------------------===//
// Utility functions.
@@ -2045,6 +2046,53 @@ static void trackRValueExpression(const ExplodedNode *InputNode, const Expr *E,
}
}
+//===----------------------------------------------------------------------===//
+// Tracker implementation
+//===----------------------------------------------------------------------===//
+
+Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) {
+ // TODO: split trackExpressionValue and FindLastStoreBRVisitor into handlers
+ // and add them here.
+}
+
+Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
+ TrackingOptions Opts) {
+ if (!E || !N)
+ return {};
+
+ const Expr *Inner = peelOffOuterExpr(E, N);
+ const ExplodedNode *LVNode = findNodeForExpression(N, Inner);
+ if (!LVNode)
+ return {};
+
+ Result CombinedResult;
+ // Iterate through the handlers in the order according to their priorities.
+ for (ExpressionHandlerPtr &Handler : ExpressionHandlers) {
+ CombinedResult.combineWith(Handler->handle(Inner, N, LVNode, Opts));
+ if (CombinedResult.WasInterrupted)
+ break;
+ }
+
+ return CombinedResult;
+}
+
+Tracker::Result Tracker::track(SVal V, const MemRegion *R, TrackingOptions Opts,
+ const StackFrameContext *Origin) {
+ // TODO: support this operation after dismantling FindLastStoreBRVisitor
+ return {};
+}
+
+PathDiagnosticPieceRef Tracker::handle(StoreInfo SI, 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 the handler produced a non-null piece, return it.
+ // There is no need in asking other handlers.
+ return Result;
+ }
+ return {};
+}
+
bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
const Expr *E,
PathSensitiveBugReport &report,
More information about the cfe-commits
mailing list