r368777 - [analyzer][NFC] Prepare visitors for different tracking kinds
Kristof Umann via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 13 17:48:57 PDT 2019
Author: szelethus
Date: Tue Aug 13 17:48:57 2019
New Revision: 368777
URL: http://llvm.org/viewvc/llvm-project?rev=368777&view=rev
Log:
[analyzer][NFC] Prepare visitors for different tracking kinds
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.
There are a variety of heuristics we discussed on the mailing list[1] 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
several visitors as a non-optional argument, and moves some functions around to
make the code a little more coherent.
[1] http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html
Differential Revision: https://reviews.llvm.org/D64270
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h?rev=368777&r1=368776&r2=368777&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h Tue Aug 13 17:48:57 2019
@@ -85,6 +85,40 @@ public:
const BugReport &BR);
};
+namespace bugreporter {
+
+/// Specifies the type of tracking for an expression.
+enum class TrackingKind {
+ /// Default tracking kind -- specifies that as much information should be
+ /// gathered about the tracked expression value as possible.
+ Thorough,
+ /// Specifies that a more moderate tracking should be used for the expression
+ /// value. This will essentially make sure that functions relevant to the it
+ /// aren't pruned, but otherwise relies on the user reading the code or
+ /// following the arrows.
+ Condition
+};
+
+/// 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,
+ TrackingKind TKind = TrackingKind::Thorough,
+ bool EnableNullFPSuppression = true);
+
+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 {
@@ -96,15 +130,28 @@ class FindLastStoreBRVisitor final : pub
/// 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;
@@ -347,27 +394,6 @@ public:
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
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp?rev=368777&r1=368776&r2=368777&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MIGChecker.cpp Tue Aug 13 17:48:57 2019
@@ -282,7 +282,8 @@ void MIGChecker::checkReturnAux(const Re
N);
R->addRange(RS->getSourceRange());
- bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false);
+ bugreporter::trackExpressionValue(N, RS->getRetValue(), *R,
+ bugreporter::TrackingKind::Thorough, false);
C.emitReport(std::move(R));
}
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp?rev=368777&r1=368776&r2=368777&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp Tue Aug 13 17:48:57 2019
@@ -146,8 +146,8 @@ void ObjCContainersChecker::checkPreStmt
initBugType();
auto R = llvm::make_unique<BugReport>(*BT, "Index is out of bounds", N);
R->addRange(IdxExpr->getSourceRange());
- bugreporter::trackExpressionValue(N, IdxExpr, *R,
- /*EnableNullFPSuppression=*/false);
+ bugreporter::trackExpressionValue(
+ N, IdxExpr, *R, bugreporter::TrackingKind::Thorough, false);
C.emitReport(std::move(R));
return;
}
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp?rev=368777&r1=368776&r2=368777&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp Tue Aug 13 17:48:57 2019
@@ -87,7 +87,8 @@ UndefCapturedBlockVarChecker::checkPostS
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::Thorough));
R->disablePathPruning();
// need location of block
C.emitReport(std::move(R));
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=368777&r1=368776&r2=368777&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Tue Aug 13 17:48:57 2019
@@ -851,12 +851,13 @@ class ReturnVisitor : public BugReporter
bool EnableNullFPSuppression;
bool ShouldInvalidate = true;
AnalyzerOptions& Options;
+ bugreporter::TrackingKind TKind;
public:
ReturnVisitor(const StackFrameContext *Frame, bool Suppressed,
- AnalyzerOptions &Options)
+ AnalyzerOptions &Options, bugreporter::TrackingKind TKind)
: CalleeSFC(Frame), EnableNullFPSuppression(Suppressed),
- Options(Options) {}
+ Options(Options), TKind(TKind) {}
static void *getTag() {
static int Tag = 0;
@@ -878,7 +879,8 @@ public:
/// bug report, so it can print a note later.
static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
BugReport &BR,
- bool InEnableNullFPSuppression) {
+ bool InEnableNullFPSuppression,
+ bugreporter::TrackingKind TKind) {
if (!CallEvent::isCallStmt(S))
return;
@@ -951,7 +953,7 @@ public:
BR.addVisitor(llvm::make_unique<ReturnVisitor>(CalleeContext,
EnableNullFPSuppression,
- Options));
+ Options, TKind));
}
PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N,
@@ -999,8 +1001,9 @@ public:
RetE = RetE->IgnoreParenCasts();
- // If we're returning 0, we should track where that 0 came from.
- bugreporter::trackExpressionValue(N, RetE, BR, EnableNullFPSuppression);
+ // Let's track the return value.
+ bugreporter::trackExpressionValue(
+ N, RetE, BR, TKind, EnableNullFPSuppression);
// Build an appropriate message based on the return value.
SmallString<64> Msg;
@@ -1115,7 +1118,7 @@ public:
if (!State->isNull(*ArgV).isConstrainedTrue())
continue;
- if (bugreporter::trackExpressionValue(N, ArgE, BR, EnableNullFPSuppression))
+ if (trackExpressionValue(N, ArgE, BR, TKind, EnableNullFPSuppression))
ShouldInvalidate = false;
// If we /can't/ track the null pointer, we should err on the side of
@@ -1159,9 +1162,45 @@ void FindLastStoreBRVisitor::Profile(llv
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) {
@@ -1332,7 +1371,7 @@ FindLastStoreBRVisitor::VisitNode(const
// 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();
}
@@ -1397,8 +1436,8 @@ FindLastStoreBRVisitor::VisitNode(const
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
- bugreporter::trackExpressionValue(StoreSite, InitE, BR,
- EnableNullFPSuppression);
+ bugreporter::trackExpressionValue(
+ StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
}
// Okay, we've found the binding. Emit an appropriate message.
@@ -1426,7 +1465,7 @@ FindLastStoreBRVisitor::VisitNode(const
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));
}
}
}
@@ -1724,7 +1763,8 @@ PathDiagnosticPieceRef TrackControlDepen
// expression, hence the BugReport level set.
if (BR.addTrackedCondition(N)) {
bugreporter::trackExpressionValue(
- N, Condition, BR, /*EnableNullFPSuppression=*/false);
+ N, Condition, BR, bugreporter::TrackingKind::Condition,
+ /*EnableNullFPSuppression=*/false);
return constructDebugPieceForTrackedCondition(Condition, N, BRC);
}
}
@@ -1838,7 +1878,9 @@ static const ExplodedNode* findNodeForEx
bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
const Expr *E, BugReport &report,
+ bugreporter::TrackingKind TKind,
bool EnableNullFPSuppression) {
+
if (!E || !InputNode)
return false;
@@ -1862,12 +1904,13 @@ bool bugreporter::trackExpressionValue(c
// At this point in the path, the receiver should be live since we are at the
// message send expr. If it is nil, start tracking it.
if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode))
- trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
+ trackExpressionValue(
+ LVNode, Receiver, report, TKind, EnableNullFPSuppression);
// Track the index if this is an array subscript.
if (const auto *Arr = dyn_cast<ArraySubscriptExpr>(Inner))
trackExpressionValue(
- LVNode, Arr->getIdx(), report, /*EnableNullFPSuppression*/ false);
+ LVNode, Arr->getIdx(), report, TKind, /*EnableNullFPSuppression*/false);
// See if the expression we're interested refers to a variable.
// If so, we can track both its contents and constraints on its value.
@@ -1883,7 +1926,7 @@ bool bugreporter::trackExpressionValue(c
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.
@@ -1920,7 +1963,7 @@ bool bugreporter::trackExpressionValue(c
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
- *KV, R, EnableNullFPSuppression));
+ *KV, R, EnableNullFPSuppression, TKind));
return true;
}
}
@@ -1930,7 +1973,7 @@ bool bugreporter::trackExpressionValue(c
SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
ReturnVisitor::addVisitorIfNecessary(
- LVNode, Inner, report, EnableNullFPSuppression);
+ LVNode, Inner, report, EnableNullFPSuppression, TKind);
// Is it a symbolic value?
if (auto L = V.getAs<loc::MemRegionVal>()) {
@@ -1959,7 +2002,7 @@ bool bugreporter::trackExpressionValue(c
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)) {
@@ -2017,8 +2060,9 @@ PathDiagnosticPieceRef NilReceiverBRVisi
// 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,
- /*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());
@@ -2026,45 +2070,6 @@ PathDiagnosticPieceRef NilReceiverBRVisi
}
//===----------------------------------------------------------------------===//
-// 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.
//===----------------------------------------------------------------------===//
More information about the cfe-commits
mailing list