Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, Charusso,
baloghadamsoftware, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, whisperity.
When we're tracking a variable that is responsible for a null pointer
dereference or some other sinister programming error, we of course would like
to gather as much information why we think that the variable has that specific
value as possible. However, the newly introduced condition tracking shows that
tracking all values this thoroughly could easily cause an intolerable growth in
the bug report's length.
Take, for instance the following report as an example: BEFORE condition
tracking
<http://cc.elte.hu:15001/Default/#is-unique=on&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=07aee7deb54777a4042572f86714de35&report=4364&subtab=07aee7deb54777a4042572f86714de35>,
AFTER condition tracking
<http://cc.elte.hu:15001/Default/#is-unique=on&run=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&review-status=Confirmed&review-status=False%20positive&review-status=Intentional&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=LLVM%2FClang%2FClang-tools-extra%20AFTER%20tracking%20conditions&reportHash=07aee7deb54777a4042572f86714de35&report=3037&subtab=07aee7deb54777a4042572f86714de35>.
Having an already lengthy, bug report with 18 events grew to 45 is inexcusable.
There are a variety of heuristics we discussed on the mailing list
<http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html> to combat this,
all of them requiring to differentiate in between tracking a "regular value"
and a "condition".
This patch introduces the new `bugreporter::TrackingKind` enum, adds it to
`FindLastStoreBRVisitor` as a non-optional argument, and moves some functions
around to make the code a little more coherent.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D64270
Files:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -71,6 +71,20 @@
// Utility functions.
//===----------------------------------------------------------------------===//
+/// Implementation function for trackExpressionValue().
+static bool trackExpressionValue(
+ const ExplodedNode *InputNode,
+ const Expr *E, BugReport &report,
+ bool EnableNullFPSuppression,
+ bugreporter::TrackingKind TKind);
+
+bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
+ const Expr *E, BugReport &report,
+ bool EnableNullFPSuppression) {
+ return ::trackExpressionValue(InputNode, E, report, EnableNullFPSuppression,
+ TrackingKind::ThoroughTracking);
+}
+
static const Expr *peelOffPointerArithmetic(const BinaryOperator *B) {
if (B->isAdditiveOp() && B->getType()->isPointerType()) {
if (B->getLHS()->getType()->isPointerType()) {
@@ -1141,9 +1155,45 @@
ID.AddPointer(&tag);
ID.AddPointer(R);
ID.Add(V);
+ ID.AddInteger(static_cast<int>(TKind));
ID.AddBoolean(EnableNullFPSuppression);
}
+void FindLastStoreBRVisitor::registerStatementVarDecls(
+ BugReport &BR, const Stmt *S, bool EnableNullFPSuppression,
+ TrackingKind TKind) {
+
+ const ExplodedNode *N = BR.getErrorNode();
+ std::deque<const Stmt *> WorkList;
+ WorkList.push_back(S);
+
+ while (!WorkList.empty()) {
+ const Stmt *Head = WorkList.front();
+ WorkList.pop_front();
+
+ ProgramStateManager &StateMgr = N->getState()->getStateManager();
+
+ if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
+ if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
+ const VarRegion *R =
+ StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());
+
+ // What did we load?
+ SVal V = N->getSVal(S);
+
+ if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
+ // Register a new visitor with the BugReport.
+ BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
+ V.castAs<KnownSVal>(), R, EnableNullFPSuppression, TKind));
+ }
+ }
+ }
+
+ for (const Stmt *SubStmt : Head->children())
+ WorkList.push_back(SubStmt);
+ }
+}
+
/// Returns true if \p N represents the DeclStmt declaring and initializing
/// \p VR.
static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) {
@@ -1314,7 +1364,7 @@
// should track the initializer expression.
if (Optional<PostInitializer> PIP = Pred->getLocationAs<PostInitializer>()) {
const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue();
- if (FieldReg && FieldReg == R) {
+ if (FieldReg == R) {
StoreSite = Pred;
InitE = PIP->getInitializer()->getInit();
}
@@ -1412,7 +1462,7 @@
if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *KV, OriginalR, EnableNullFPSuppression));
+ *KV, OriginalR, EnableNullFPSuppression, TKind));
}
}
}
@@ -1713,8 +1763,9 @@
// isn't sufficient, because a new visitor is created for each tracked
// expression, hence the BugReport level set.
if (BR.addTrackedCondition(N)) {
- bugreporter::trackExpressionValue(
- N, Condition, BR, /*EnableNullFPSuppression=*/false);
+ trackExpressionValue(
+ N, Condition, BR, /*EnableNullFPSuppression=*/false,
+ bugreporter::TrackingKind::ConditionTracking);
return constructDebugPieceForTrackedCondition(Condition, N, BRC);
}
}
@@ -1826,9 +1877,14 @@
return N;
}
-bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
- const Expr *E, BugReport &report,
- bool EnableNullFPSuppression) {
+static bool trackExpressionValue(
+ const ExplodedNode *InputNode,
+ const Expr *E, BugReport &report,
+ bool EnableNullFPSuppression,
+ bugreporter::TrackingKind TKind /* = TrackingKind::ThoroughTracking*/) {
+
+ using namespace bugreporter;
+
if (!E || !InputNode)
return false;
@@ -1873,7 +1929,7 @@
if (RR && !LVIsNull)
if (auto KV = LVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *KV, RR, EnableNullFPSuppression));
+ *KV, RR, EnableNullFPSuppression, TKind));
// In case of C++ references, we want to differentiate between a null
// reference and reference to null pointer.
@@ -1910,7 +1966,7 @@
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *KV, R, EnableNullFPSuppression));
+ *KV, R, EnableNullFPSuppression, TKind));
return true;
}
}
@@ -1948,7 +2004,7 @@
if (CanDereference)
if (auto KV = RVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *KV, L->getRegion(), EnableNullFPSuppression));
+ *KV, L->getRegion(), EnableNullFPSuppression, TKind));
const MemRegion *RegionRVal = RVal.getAsRegion();
if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
@@ -2014,45 +2070,6 @@
return std::make_shared<PathDiagnosticEventPiece>(L, OS.str());
}
-//===----------------------------------------------------------------------===//
-// Implementation of FindLastStoreBRVisitor.
-//===----------------------------------------------------------------------===//
-
-// Registers every VarDecl inside a Stmt with a last store visitor.
-void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR,
- const Stmt *S,
- bool EnableNullFPSuppression) {
- const ExplodedNode *N = BR.getErrorNode();
- std::deque<const Stmt *> WorkList;
- WorkList.push_back(S);
-
- while (!WorkList.empty()) {
- const Stmt *Head = WorkList.front();
- WorkList.pop_front();
-
- ProgramStateManager &StateMgr = N->getState()->getStateManager();
-
- if (const auto *DR = dyn_cast<DeclRefExpr>(Head)) {
- if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
- const VarRegion *R =
- StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext());
-
- // What did we load?
- SVal V = N->getSVal(S);
-
- if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
- // Register a new visitor with the BugReport.
- BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- V.castAs<KnownSVal>(), R, EnableNullFPSuppression));
- }
- }
- }
-
- for (const Stmt *SubStmt : Head->children())
- WorkList.push_back(SubStmt);
- }
-}
-
//===----------------------------------------------------------------------===//
// Visitor that tries to report interesting diagnostics from conditions.
//===----------------------------------------------------------------------===//
Index: clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
@@ -87,7 +87,8 @@
if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD))
R->addRange(Ex->getSourceRange());
R->addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *V, VR, /*EnableNullFPSuppression*/ false));
+ *V, VR, /*EnableNullFPSuppression*/ false,
+ bugreporter::TrackingKind::ThoroughTracking));
R->disablePathPruning();
// need location of block
C.emitReport(std::move(R));
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
@@ -83,6 +83,36 @@
BugReport &BR);
};
+namespace bugreporter {
+
+/// Attempts to add visitors to track expression value back to its point of
+/// origin.
+///
+/// \param N A node "downstream" from the evaluation of the statement.
+/// \param E The expression value which we are tracking
+/// \param R The bug report to which visitors should be attached.
+/// \param EnableNullFPSuppression Whether we should employ false positive
+/// suppression (inlined defensive checks, returned null).
+///
+/// \return Whether or not the function was able to add visitors for this
+/// statement. Note that returning \c true does not actually imply
+/// that any visitors were added.
+bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
+ bool EnableNullFPSuppression = true);
+
+/// Specifies the type of tracking for an expression. For instance, we'd like to
+/// gather far more information about a variable found to be a cause of a null
+/// pointer dereference, while tracking a condition to that extent would pollute
+/// the bug report without much of an added value.
+enum class TrackingKind {
+ ThoroughTracking,
+ ConditionTracking
+};
+
+const Expr *getDerefExpr(const Stmt *S);
+
+} // namespace bugreporter
+
/// Finds last store into the given region,
/// which is different from a given symbolic value.
class FindLastStoreBRVisitor final : public BugReporterVisitor {
@@ -94,15 +124,28 @@
/// bug, we are going to employ false positive suppression.
bool EnableNullFPSuppression;
+ using TrackingKind = bugreporter::TrackingKind;
+ TrackingKind TKind;
+
public:
/// Creates a visitor for every VarDecl inside a Stmt and registers it with
/// the BugReport.
static void registerStatementVarDecls(BugReport &BR, const Stmt *S,
- bool EnableNullFPSuppression);
-
+ bool EnableNullFPSuppression,
+ TrackingKind TKind);
+
+ /// \param V We're searching for the store where \c R received this value.
+ /// \param R The region we're tracking.
+ /// \param EnableNullFPSuppression Whether we should employ false positive
+ /// suppression (inlined defensive checks, returned null).
+ /// \param TKind May limit the amount of notes added to the bug report.
FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R,
- bool InEnableNullFPSuppression)
- : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression) {}
+ bool InEnableNullFPSuppression,
+ TrackingKind TKind)
+ : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression),
+ TKind(TKind) {
+ assert(R);
+ }
void Profile(llvm::FoldingSetNodeID &ID) const override;
@@ -345,27 +388,6 @@
BugReport &R) override;
};
-namespace bugreporter {
-
-/// Attempts to add visitors to track expression value back to its point of
-/// origin.
-///
-/// \param N A node "downstream" from the evaluation of the statement.
-/// \param E The expression value which we are tracking
-/// \param R The bug report to which visitors should be attached.
-/// \param EnableNullFPSuppression Whether we should employ false positive
-/// suppression (inlined defensive checks, returned null).
-///
-/// \return Whether or not the function was able to add visitors for this
-/// statement. Note that returning \c true does not actually imply
-/// that any visitors were added.
-bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R,
- bool EnableNullFPSuppression = true);
-
-const Expr *getDerefExpr(const Stmt *S);
-
-} // namespace bugreporter
-
} // namespace ento
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
