r343159 - [analyzer] [NFC] Heavy refactoring of trackNullOrUndefValue
George Karpenkov via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 26 18:45:58 PDT 2018
Author: george.karpenkov
Date: Wed Sep 26 18:45:57 2018
New Revision: 343159
URL: http://llvm.org/viewvc/llvm-project?rev=343159&view=rev
Log:
[analyzer] [NFC] Heavy refactoring of trackNullOrUndefValue
Differential Revision: https://reviews.llvm.org/D52519
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
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=343159&r1=343158&r2=343159&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h Wed Sep 26 18:45:57 2018
@@ -363,9 +363,6 @@ namespace bugreporter {
/// \param N A node "downstream" from the evaluation of the statement.
/// \param S The statement whose value is null or undefined.
/// \param R The bug report to which visitors should be attached.
-/// \param IsArg Whether the statement is an argument to an inlined function.
-/// If this is the case, \p N \em must be the CallEnter node for
-/// the function.
/// \param EnableNullFPSuppression Whether we should employ false positive
/// suppression (inlined defensive checks, returned null).
///
@@ -373,7 +370,6 @@ namespace bugreporter {
/// statement. Note that returning \c true does not actually imply
/// that any visitors were added.
bool trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R,
- bool IsArg = false,
bool EnableNullFPSuppression = true);
const Expr *getDerefExpr(const Stmt *S);
Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=343159&r1=343158&r2=343159&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Wed Sep 26 18:45:57 2018
@@ -888,8 +888,7 @@ public:
RetE = RetE->IgnoreParenCasts();
// If we're returning 0, we should track where that 0 came from.
- bugreporter::trackNullOrUndefValue(N, RetE, BR, /*IsArg*/ false,
- EnableNullFPSuppression);
+ bugreporter::trackNullOrUndefValue(N, RetE, BR, EnableNullFPSuppression);
// Build an appropriate message based on the return value.
SmallString<64> Msg;
@@ -984,7 +983,7 @@ public:
if (!State->isNull(*ArgV).isConstrainedTrue())
continue;
- if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true,
+ if (bugreporter::trackNullOrUndefValue(N, ArgE, BR,
EnableNullFPSuppression))
ShouldInvalidate = false;
@@ -1264,7 +1263,7 @@ FindLastStoreBRVisitor::VisitNode(const
V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) {
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
- bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam,
+ bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR,
EnableNullFPSuppression);
}
ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(),
@@ -1516,6 +1515,8 @@ static const MemRegion *getLocationRegio
return nullptr;
}
+/// \return A subexpression of {@code Ex} which represents the
+/// expression-of-interest.
static const Expr *peelOffOuterExpr(const Expr *Ex,
const ExplodedNode *N) {
Ex = Ex->IgnoreParenCasts();
@@ -1560,125 +1561,73 @@ static const Expr *peelOffOuterExpr(cons
if (const Expr *SubEx = peelOffPointerArithmetic(BO))
return peelOffOuterExpr(SubEx, N);
- if (auto *UO = dyn_cast<UnaryOperator>(Ex))
+ if (auto *UO = dyn_cast<UnaryOperator>(Ex)) {
if (UO->getOpcode() == UO_LNot)
return peelOffOuterExpr(UO->getSubExpr(), N);
- return Ex;
-}
+ // FIXME: There's a hack in our Store implementation that always computes
+ // field offsets around null pointers as if they are always equal to 0.
+ // The idea here is to report accesses to fields as null dereferences
+ // even though the pointer value that's being dereferenced is actually
+ // the offset of the field rather than exactly 0.
+ // See the FIXME in StoreManager's getLValueFieldOrIvar() method.
+ // This code interacts heavily with this hack; otherwise the value
+ // would not be null at all for most fields, so we'd be unable to track it.
+ if (UO->getOpcode() == UO_AddrOf && UO->getSubExpr()->isLValue())
+ if (const Expr *DerefEx = bugreporter::getDerefExpr(UO->getSubExpr()))
+ return peelOffOuterExpr(DerefEx, N);
+ }
-/// Walk through nodes until we get one that matches the statement exactly.
-/// Alternately, if we hit a known lvalue for the statement, we know we've
-/// gone too far (though we can likely track the lvalue better anyway).
-static const ExplodedNode* findNodeForStatement(const ExplodedNode *N,
- const Stmt *S,
- const Expr *Inner) {
- do {
- const ProgramPoint &pp = N->getLocation();
- if (auto ps = pp.getAs<StmtPoint>()) {
- if (ps->getStmt() == S || ps->getStmt() == Inner)
- break;
- } else if (auto CEE = pp.getAs<CallExitEnd>()) {
- if (CEE->getCalleeContext()->getCallSite() == S ||
- CEE->getCalleeContext()->getCallSite() == Inner)
- break;
- }
- N = N->getFirstPred();
- } while (N);
- return N;
+ return Ex;
}
/// Find the ExplodedNode where the lvalue (the value of 'Ex')
/// was computed.
static const ExplodedNode* findNodeForExpression(const ExplodedNode *N,
- const Expr *Inner) {
+ const Expr *Inner) {
while (N) {
- if (auto P = N->getLocation().getAs<PostStmt>()) {
- if (P->getStmt() == Inner)
- break;
- }
+ if (PathDiagnosticLocation::getStmt(N) == Inner)
+ return N;
N = N->getFirstPred();
}
- assert(N && "Unable to find the lvalue node.");
return N;
}
-/// Performing operator `&' on an lvalue expression is essentially a no-op.
-/// Then, if we are taking addresses of fields or elements, these are also
-/// unlikely to matter.
-static const Expr* peelOfOuterAddrOf(const Expr* Ex) {
- Ex = Ex->IgnoreParenCasts();
-
- // FIXME: There's a hack in our Store implementation that always computes
- // field offsets around null pointers as if they are always equal to 0.
- // The idea here is to report accesses to fields as null dereferences
- // even though the pointer value that's being dereferenced is actually
- // the offset of the field rather than exactly 0.
- // See the FIXME in StoreManager's getLValueFieldOrIvar() method.
- // This code interacts heavily with this hack; otherwise the value
- // would not be null at all for most fields, so we'd be unable to track it.
- if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
- if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue())
- if (const Expr *DerefEx = bugreporter::getDerefExpr(Op->getSubExpr()))
- return DerefEx;
- return Ex;
-}
-
-bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
- const Stmt *S,
- BugReport &report, bool IsArg,
+bool bugreporter::trackNullOrUndefValue(const ExplodedNode *InputNode,
+ const Stmt *InputS,
+ BugReport &report,
bool EnableNullFPSuppression) {
- if (!S || !N)
+ if (!InputS || !InputNode || !isa<Expr>(InputS))
return false;
- if (const auto *Ex = dyn_cast<Expr>(S))
- S = peelOffOuterExpr(Ex, N);
-
- const Expr *Inner = nullptr;
- if (const auto *Ex = dyn_cast<Expr>(S)) {
- Ex = peelOfOuterAddrOf(Ex);
- Ex = Ex->IgnoreParenCasts();
-
- if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex)
- || CallEvent::isCallStmt(Ex)))
- Inner = Ex;
- }
-
- if (IsArg && !Inner) {
- assert(N->getLocation().getAs<CallEnter>() && "Tracking arg but not at call");
- } else {
- N = findNodeForStatement(N, S, Inner);
- if (!N)
- return false;
- }
+ const Expr *Inner = peelOffOuterExpr(cast<Expr>(InputS), InputNode);
+ const ExplodedNode *LVNode = findNodeForExpression(InputNode, Inner);
+ if (!LVNode)
+ return false;
- ProgramStateRef state = N->getState();
+ ProgramStateRef LVState = LVNode->getState();
// The message send could be nil due to the receiver being nil.
// 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(S, N))
- trackNullOrUndefValue(N, Receiver, report, /* IsArg=*/ false,
- EnableNullFPSuppression);
+ if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode))
+ trackNullOrUndefValue(LVNode, Receiver, report, EnableNullFPSuppression);
// See if the expression we're interested refers to a variable.
// If so, we can track both its contents and constraints on its value.
if (Inner && ExplodedGraph::isInterestingLValueExpr(Inner)) {
- const ExplodedNode *LVNode = findNodeForExpression(N, Inner);
- ProgramStateRef LVState = LVNode->getState();
SVal LVal = LVNode->getSVal(Inner);
- const MemRegion *RR = getLocationRegionIfReference(Inner, N);
+ const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode);
bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue();
// If this is a C++ reference to a null pointer, we are tracking the
// pointer. In addition, we should find the store at which the reference
// got initialized.
- if (RR && !LVIsNull) {
+ if (RR && !LVIsNull)
if (auto KV = LVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, RR, EnableNullFPSuppression));
- }
// In case of C++ references, we want to differentiate between a null
// reference and reference to null pointer.
@@ -1688,7 +1637,6 @@ bool bugreporter::trackNullOrUndefValue(
LVNode->getSVal(Inner).getAsRegion();
if (R) {
- ProgramStateRef S = N->getState();
// Mark both the variable region and its contents as interesting.
SVal V = LVState->getRawSVal(loc::MemRegionVal(R));
@@ -1696,9 +1644,8 @@ bool bugreporter::trackNullOrUndefValue(
llvm::make_unique<NoStoreFuncVisitor>(cast<SubRegion>(R)));
MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary(
- N, R, EnableNullFPSuppression, report, V);
+ LVNode, R, EnableNullFPSuppression, report, V);
- report.markInteresting(R);
report.markInteresting(V);
report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(R));
@@ -1708,14 +1655,12 @@ bool bugreporter::trackNullOrUndefValue(
V.castAs<DefinedSVal>(), false));
// Add visitor, which will suppress inline defensive checks.
- if (auto DV = V.getAs<DefinedSVal>()) {
+ if (auto DV = V.getAs<DefinedSVal>())
if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
- EnableNullFPSuppression) {
+ EnableNullFPSuppression)
report.addVisitor(
llvm::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
- LVNode));
- }
- }
+ LVNode));
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
@@ -1726,41 +1671,35 @@ bool bugreporter::trackNullOrUndefValue(
// If the expression is not an "lvalue expression", we can still
// track the constraints on its contents.
- SVal V = state->getSValAsScalarOrLoc(S, N->getLocationContext());
+ SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
+
+ ReturnVisitor::addVisitorIfNecessary(
+ LVNode, Inner, report, EnableNullFPSuppression);
- // If the value came from an inlined function call, we should at least make
- // sure that function isn't pruned in our output.
- if (const auto *E = dyn_cast<Expr>(S))
- S = E->IgnoreParenCasts();
-
- ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression);
-
- // Uncomment this to find cases where we aren't properly getting the
- // base value that was dereferenced.
- // assert(!V.isUnknownOrUndef());
// Is it a symbolic value?
if (auto L = V.getAs<loc::MemRegionVal>()) {
report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
- // At this point we are dealing with the region's LValue.
- // However, if the rvalue is a symbolic region, we should track it as well.
- // Try to use the correct type when looking up the value.
- SVal RVal;
- if (const auto *E = dyn_cast<Expr>(S))
- RVal = state->getRawSVal(L.getValue(), E->getType());
- else
- RVal = state->getSVal(L->getRegion());
-
// FIXME: this is a hack for fixing a later crash when attempting to
// dereference a void* pointer.
// We should not try to dereference pointers at all when we don't care
// what is written inside the pointer.
- bool ShouldFindLastStore = true;
+ bool CanDereference = true;
if (const auto *SR = dyn_cast<SymbolicRegion>(L->getRegion()))
if (SR->getSymbol()->getType()->getPointeeType()->isVoidType())
- ShouldFindLastStore = false;
+ CanDereference = false;
+
+ // At this point we are dealing with the region's LValue.
+ // However, if the rvalue is a symbolic region, we should track it as well.
+ // Try to use the correct type when looking up the value.
+ SVal RVal;
+ if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
+ RVal = LVState->getRawSVal(L.getValue(), Inner->getType());
+ } else if (CanDereference) {
+ RVal = LVState->getSVal(L->getRegion());
+ }
- if (ShouldFindLastStore)
+ if (CanDereference)
if (auto KV = RVal.getAs<KnownSVal>())
report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
*KV, L->getRegion(), EnableNullFPSuppression));
@@ -1818,7 +1757,7 @@ NilReceiverBRVisitor::VisitNode(const Ex
// 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::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false,
+ bugreporter::trackNullOrUndefValue(N, Receiver, BR,
/*EnableNullFPSuppression*/ false);
// Issue a message saying that the method was skipped.
PathDiagnosticLocation L(Receiver, BRC.getSourceManager(),
More information about the cfe-commits
mailing list