Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso,
a_sidorin, rnkovacs, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, whisperity.
Well, what is says on the tin I guess!
Some more changes:
- Move `isInevitablySinking()` from `BugReporter.cpp` to `CFGBlock`'s interface
- Rename and move `findBlockForNode()` from `BugReporter.cpp` to
`ExplodedNode::getCFGBlock()`
- Add some testcases, but are these assert implementations esoteric enough?
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D65287
Files:
clang/include/clang/Analysis/CFG.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
clang/lib/Analysis/CFG.cpp
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
clang/test/Analysis/track-control-dependency-conditions.cpp
Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===================================================================
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -428,3 +428,116 @@
}
} // end of namespace unimportant_write_before_collapse_point
+
+namespace dont_track_assertlike_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+ unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr) ? (void)(0) : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+
+void bar() {
+ cond1 = getInt();
+}
+
+void f(int flag) {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+ flag = getInt();
+
+ bar();
+ assert(cond1); // expected-note{{Assuming 'cond1' is not equal to 0}}
+ // expected-note@-1{{'?' condition is true}}
+
+ if (flag) // expected-note{{'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ // debug-note@-2{{Tracking condition 'flag'}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_conditions
+
+namespace dont_track_assertlike_and_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+ unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr) ? (void)(0) : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+int cond2;
+
+void bar() {
+ cond1 = getInt();
+ cond2 = getInt();
+}
+
+void f(int flag) {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+ flag = getInt();
+
+ bar();
+ assert(cond1 && cond2);
+ // expected-note@-1{{Assuming 'cond1' is not equal to 0}}
+ // expected-note@-2{{Assuming 'cond2' is not equal to 0}}
+ // expected-note@-3{{'?' condition is true}}
+ // expected-note@-4{{Left side of '&&' is true}}
+
+ if (flag) // expected-note{{'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ // debug-note@-2{{Tracking condition 'flag'}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_and_conditions
+
+namespace dont_track_assertlike_or_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+ unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr) ? (void)(0) : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+int cond2;
+
+void bar() {
+ cond1 = getInt();
+ cond2 = getInt();
+}
+
+void f(int flag) {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+ flag = getInt();
+
+ bar();
+ assert(cond1 || cond2);
+ // expected-note@-1{{Assuming 'cond1' is not equal to 0}}
+ // expected-note@-2{{Left side of '||' is true}}
+
+ if (flag) // expected-note{{'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ // debug-note@-2{{Tracking condition 'flag'}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_or_conditions
Index: clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -16,6 +16,7 @@
#include "clang/AST/ExprObjC.h"
#include "clang/AST/ParentMap.h"
#include "clang/AST/Stmt.h"
+#include "clang/Analysis/CFGStmtMap.h"
#include "clang/Analysis/ProgramPoint.h"
#include "clang/Analysis/Support/BumpVector.h"
#include "clang/Basic/LLVM.h"
@@ -292,6 +293,19 @@
getFirstPred()->succ_size() == 1;
}
+const CFGBlock *ExplodedNode::getCFGBlock() const {
+ ProgramPoint P = getLocation();
+ if (auto BEP = P.getAs<BlockEntrance>())
+ return BEP->getBlock();
+
+ // Find the node's current statement in the CFG.
+ if (const Stmt *S = PathDiagnosticLocation::getStmt(this))
+ return getLocationContext()->getAnalysisDeclContext()
+ ->getCFGStmtMap()->getBlock(S);
+
+ return nullptr;
+}
+
ExplodedNode *ExplodedGraph::getNode(const ProgramPoint &L,
ProgramStateRef State,
bool IsSink,
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1710,18 +1710,6 @@
};
} // end of anonymous namespace
-static CFGBlock *GetRelevantBlock(const ExplodedNode *Node) {
- if (auto SP = Node->getLocationAs<StmtPoint>()) {
- const Stmt *S = SP->getStmt();
- assert(S);
-
- return const_cast<CFGBlock *>(Node->getLocationContext()
- ->getAnalysisDeclContext()->getCFGStmtMap()->getBlock(S));
- }
-
- return nullptr;
-}
-
static std::shared_ptr<PathDiagnosticEventPiece>
constructDebugPieceForTrackedCondition(const Expr *Cond,
const ExplodedNode *N,
@@ -1742,6 +1730,44 @@
(Twine() + "Tracking condition '" + ConditionText + "'").str());
}
+static bool isAssertlikeBlock(const CFGBlock *B, ASTContext &Context) {
+ if (B->succ_size() != 2)
+ return false;
+
+ const CFGBlock *Then = B->succ_begin()->getReachableBlock();
+ const CFGBlock *Else = (B->succ_begin() + 1)->getReachableBlock();
+
+ if (!Then || !Else)
+ return false;
+
+ if (Then->isInevitablySinking() || Else->isInevitablySinking())
+ return true;
+
+ // For the following condition the following CFG would be built:
+ //
+ //
+ // [B1] -> [B2] -> [B3] -> [sink]
+ // assert(A && B || C); \ \ \
+ // -------------------> [go on with the execution]
+ //
+ // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block
+ // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we
+ // reached the end of the condition!
+ if (const Stmt *ElseCond = Else->getTerminatorCondition()) {
+ const Stmt *OuterCond = B->getTerminatorCondition();
+ assert(OuterCond);
+
+ using namespace ast_matchers;
+ auto IsSubStmtOfCondition =
+ stmt(forEachDescendant(equalsNode(OuterCond))).bind("s");
+
+ if (selectFirst<Stmt>("s", match(IsSubStmtOfCondition, *ElseCond, Context)))
+ return isAssertlikeBlock(Else, Context);
+ }
+
+ return false;
+}
+
std::shared_ptr<PathDiagnosticPiece>
TrackControlDependencyCondBRVisitor::VisitNode(const ExplodedNode *N,
BugReporterContext &BRC,
@@ -1750,18 +1776,21 @@
if (Origin->getStackFrame() != N->getStackFrame())
return nullptr;
- CFGBlock *NB = GetRelevantBlock(N);
+ CFGBlock *NB = const_cast<CFGBlock *>(N->getCFGBlock());
// Skip if we already inspected this block.
if (!VisitedBlocks.insert(NB).second)
return nullptr;
- CFGBlock *OriginB = GetRelevantBlock(Origin);
+ CFGBlock *OriginB = const_cast<CFGBlock *>(Origin->getCFGBlock());
// TODO: Cache CFGBlocks for each ExplodedNode.
if (!OriginB || !NB)
return nullptr;
+ if (isAssertlikeBlock(NB, BRC.getASTContext()))
+ return nullptr;
+
if (ControlDeps.isControlDependent(OriginB, NB)) {
if (const Expr *Condition = NB->getLastCondition()) {
// Keeping track of the already tracked conditions on a visitor level
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2758,90 +2758,6 @@
} // namespace
-static const CFGBlock *findBlockForNode(const ExplodedNode *N) {
- ProgramPoint P = N->getLocation();
- if (auto BEP = P.getAs<BlockEntrance>())
- return BEP->getBlock();
-
- // Find the node's current statement in the CFG.
- if (const Stmt *S = PathDiagnosticLocation::getStmt(N))
- return N->getLocationContext()->getAnalysisDeclContext()
- ->getCFGStmtMap()->getBlock(S);
-
- return nullptr;
-}
-
-// Returns true if by simply looking at the block, we can be sure that it
-// results in a sink during analysis. This is useful to know when the analysis
-// was interrupted, and we try to figure out if it would sink eventually.
-// There may be many more reasons why a sink would appear during analysis
-// (eg. checkers may generate sinks arbitrarily), but here we only consider
-// sinks that would be obvious by looking at the CFG.
-static bool isImmediateSinkBlock(const CFGBlock *Blk) {
- if (Blk->hasNoReturnElement())
- return true;
-
- // FIXME: Throw-expressions are currently generating sinks during analysis:
- // they're not supported yet, and also often used for actually terminating
- // the program. So we should treat them as sinks in this analysis as well,
- // at least for now, but once we have better support for exceptions,
- // we'd need to carefully handle the case when the throw is being
- // immediately caught.
- if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
- if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
- if (isa<CXXThrowExpr>(StmtElm->getStmt()))
- return true;
- return false;
- }))
- return true;
-
- return false;
-}
-
-// Returns true if by looking at the CFG surrounding the node's program
-// point, we can be sure that any analysis starting from this point would
-// eventually end with a sink. We scan the child CFG blocks in a depth-first
-// manner and see if all paths eventually end up in an immediate sink block.
-static bool isInevitablySinking(const ExplodedNode *N) {
- const CFG &Cfg = N->getCFG();
-
- const CFGBlock *StartBlk = findBlockForNode(N);
- if (!StartBlk)
- return false;
- if (isImmediateSinkBlock(StartBlk))
- return true;
-
- llvm::SmallVector<const CFGBlock *, 32> DFSWorkList;
- llvm::SmallPtrSet<const CFGBlock *, 32> Visited;
-
- DFSWorkList.push_back(StartBlk);
- while (!DFSWorkList.empty()) {
- const CFGBlock *Blk = DFSWorkList.back();
- DFSWorkList.pop_back();
- Visited.insert(Blk);
-
- // If at least one path reaches the CFG exit, it means that control is
- // returned to the caller. For now, say that we are not sure what
- // happens next. If necessary, this can be improved to analyze
- // the parent StackFrameContext's call site in a similar manner.
- if (Blk == &Cfg.getExit())
- return false;
-
- for (const auto &Succ : Blk->succs()) {
- if (const CFGBlock *SuccBlk = Succ.getReachableBlock()) {
- if (!isImmediateSinkBlock(SuccBlk) && !Visited.count(SuccBlk)) {
- // If the block has reachable child blocks that aren't no-return,
- // add them to the worklist.
- DFSWorkList.push_back(SuccBlk);
- }
- }
- }
- }
-
- // Nothing reached the exit. It can only mean one thing: there's no return.
- return true;
-}
-
static BugReport *
FindReportInEquivalenceClass(BugReportEquivClass& EQ,
SmallVectorImpl<BugReport*> &bugReports) {
@@ -2893,8 +2809,9 @@
// to being post-dominated by a sink. This works better when the analysis
// is incomplete and we have never reached the no-return function call(s)
// that we'd inevitably bump into on this path.
- if (isInevitablySinking(errorNode))
- continue;
+ if (const CFGBlock *ErrorB = errorNode->getCFGBlock())
+ if (ErrorB->isInevitablySinking())
+ continue;
// At this point we know that 'N' is not a sink and it has at least one
// successor. Use a DFS worklist to find a non-sink end-of-path node.
Index: clang/lib/Analysis/CFG.cpp
===================================================================
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -5615,6 +5615,73 @@
Out << JsonFormat(TempOut.str(), AddQuotes);
}
+// Returns true if by simply looking at the block, we can be sure that it
+// results in a sink during analysis. This is useful to know when the analysis
+// was interrupted, and we try to figure out if it would sink eventually.
+// There may be many more reasons why a sink would appear during analysis
+// (eg. checkers may generate sinks arbitrarily), but here we only consider
+// sinks that would be obvious by looking at the CFG.
+static bool isImmediateSinkBlock(const CFGBlock *Blk) {
+ if (Blk->hasNoReturnElement())
+ return true;
+
+ // FIXME: Throw-expressions are currently generating sinks during analysis:
+ // they're not supported yet, and also often used for actually terminating
+ // the program. So we should treat them as sinks in this analysis as well,
+ // at least for now, but once we have better support for exceptions,
+ // we'd need to carefully handle the case when the throw is being
+ // immediately caught.
+ if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement &Elm) {
+ if (Optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+ if (isa<CXXThrowExpr>(StmtElm->getStmt()))
+ return true;
+ return false;
+ }))
+ return true;
+
+ return false;
+}
+
+bool CFGBlock::isInevitablySinking() const {
+ const CFG &Cfg = *getParent();
+
+ const CFGBlock *StartBlk = this;
+ if (!StartBlk)
+ return false;
+ if (isImmediateSinkBlock(StartBlk))
+ return true;
+
+ llvm::SmallVector<const CFGBlock *, 32> DFSWorkList;
+ llvm::SmallPtrSet<const CFGBlock *, 32> Visited;
+
+ DFSWorkList.push_back(StartBlk);
+ while (!DFSWorkList.empty()) {
+ const CFGBlock *Blk = DFSWorkList.back();
+ DFSWorkList.pop_back();
+ Visited.insert(Blk);
+
+ // If at least one path reaches the CFG exit, it means that control is
+ // returned to the caller. For now, say that we are not sure what
+ // happens next. If necessary, this can be improved to analyze
+ // the parent StackFrameContext's call site in a similar manner.
+ if (Blk == &Cfg.getExit())
+ return false;
+
+ for (const auto &Succ : Blk->succs()) {
+ if (const CFGBlock *SuccBlk = Succ.getReachableBlock()) {
+ if (!isImmediateSinkBlock(SuccBlk) && !Visited.count(SuccBlk)) {
+ // If the block has reachable child blocks that aren't no-return,
+ // add them to the worklist.
+ DFSWorkList.push_back(SuccBlk);
+ }
+ }
+ }
+ }
+
+ // Nothing reached the exit. It can only mean one thing: there's no return.
+ return true;
+}
+
const Expr *CFGBlock::getLastCondition() const {
// If the terminator is a temporary dtor or a virtual base, etc, we can't
// retrieve a meaningful condition, bail out.
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
@@ -153,6 +153,8 @@
CFG &getCFG() const { return *getLocationContext()->getCFG(); }
+ const CFGBlock *getCFGBlock() const;
+
ParentMap &getParentMap() const {return getLocationContext()->getParentMap();}
template <typename T>
Index: clang/include/clang/Analysis/CFG.h
===================================================================
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -855,6 +855,11 @@
void setLoopTarget(const Stmt *loopTarget) { LoopTarget = loopTarget; }
void setHasNoReturnElement() { HasNoReturnElement = true; }
+ /// Returns true if the block would eventually end with a sink. We scan the
+ /// child CFG blocks in a depth-first manner and see if all paths eventually
+ /// end up in an immediate sink block.
+ bool isInevitablySinking() const;
+
CFGTerminator getTerminator() const { return Terminator; }
Stmt *getTerminatorStmt() { return Terminator.getStmt(); }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
