Szelethus updated this revision to Diff 204973.
Szelethus added a comment.
- Rebase on the rest of the patches
- Make `TrackControlDependencyCondBRVisitor` local to `BugReporterVisitors.cpp`
- Hide the current implementation behind the off-by-default analyzer config
`"track-conditions"`
- Add two more testcases I'd like to improve on in followup patches
Do you think that this patch looks good enough now that it's off by default?
Maybe it'd be easier to develop/review followup changes separately.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62883/new/
https://reviews.llvm.org/D62883
Files:
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/analyzer-config.c
clang/test/Analysis/track-control-dependency-conditions.cpp
Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN: -analyzer-config track-conditions=true \
+// RUN: -analyzer-output=text \
+// RUN: -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+ flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+ flag = 1;
+
+ foo(); // TODO: Add nodes here about flag's value being invalidated.
+ if (flag) // expected-note {{Assuming 'flag' is 0}}
+ // expected-note@-1{{Taking false branch}}
+ x = new int;
+
+ foo(); // expected-note {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+ if (flag) // expected-note {{Assuming 'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+ flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+ int *x = 0;
+ flag = 1;
+
+ foo();
+ if (flag) // expected-note {{Assuming 'flag' is 0}}
+ // expected-note@-1{{Taking false branch}}
+ x = new int;
+
+ x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+ foo(); // expected-note {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+ if (flag) // expected-note {{Assuming 'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+ // coin() could write bar, do it's invalidated.
+ flag = coin(); // expected-note {{Value assigned to 'flag'}}
+ // expected-note@-1 {{Value assigned to 'bar'}}
+}
+
+int bar;
+
+void test() {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+ flag = 1;
+
+ foo(); // expected-note {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+ if (bar) // expected-note {{Assuming 'bar' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ if (flag) // expected-note {{Assuming 'flag' is not equal to 0}}
+ // expected-note@-1{{Taking true branch}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+ return coin(); // expected-note {{Returning value}}
+}
+
+int bar;
+
+void test() {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+ if (int flag = foo()) // expected-note {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+ // expected-note@-2{{'flag' initialized here}}
+ // expected-note@-3{{Assuming 'flag' is not equal to 0}}
+ // expected-note@-4{{Taking true branch}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+ operator bool() const { return coin(); } // expected-note {{Returning value}}
+};
+
+void test() {
+ int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+ if (ConvertsToBool()) // expected-note {{Calling 'ConvertsToBool::operator bool'}}
+ // expected-note@-1{{Returning from 'ConvertsToBool::operator bool'}}
+ // expected-note@-2{{Assuming the condition is true}}
+ // expected-note@-3{{Taking true branch}}
+ *x = 5; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
+
+} // end of namespace variable_declaration_in_condition
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -84,8 +84,9 @@
// CHECK-NEXT: suppress-c++-stdlib = true
// CHECK-NEXT: suppress-inlined-defensive-checks = true
// CHECK-NEXT: suppress-null-return-paths = true
+// CHECK-NEXT: track-conditions = false
// CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
// CHECK-NEXT: unroll-loops = false
// CHECK-NEXT: widen-loops = false
// CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 85
+// CHECK-NEXT: num-entries = 86
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -22,6 +22,7 @@
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Analysis/Analyses/Dominators.h"
#include "clang/Analysis/AnalysisDeclContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/CFGStmtMap.h"
@@ -1554,6 +1555,101 @@
return nullptr;
}
+//===----------------------------------------------------------------------===//
+// TrackControlDependencyCondBRVisitor.
+//===----------------------------------------------------------------------===//
+
+namespace {
+/// Tracks the expressions that are a control dependency of the node that was
+/// supplied to the constructor.
+/// For example:
+///
+/// cond = 1;
+/// if (cond)
+/// 10 / 0;
+///
+/// An error is emitted at line 3. This visitor realizes that the branch
+/// on line 2 is a control dependency of line 3, and tracks it's condition via
+/// trackExpressionValue().
+class TrackControlDependencyCondBRVisitor final : public BugReporterVisitor {
+ const ExplodedNode *Origin;
+ ControlDependencyCalculator ControlDeps;
+ llvm::SmallSet<const CFGBlock *, 32> VisitedBlocks;
+
+public:
+ TrackControlDependencyCondBRVisitor(const ExplodedNode *O)
+ : Origin(O), ControlDeps(&O->getCFG()) {}
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ static int x = 0;
+ ID.AddPointer(&x);
+ }
+
+ std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC,
+ BugReport &BR) override;
+};
+} // 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 const Expr *getTerminatorCondition(CFGBlock *B) {
+ // If the terminator is a temporary dtor or a virtual base, etc, we can't
+ // retrieve a meaningful condition, bail out.
+ if (B->rbegin()->getKind() != CFGElement::Kind::Statement)
+ return nullptr;
+
+ // This should be the condition of the terminator block.
+ const Stmt *S = B->rbegin()->castAs<CFGStmt>().getStmt();
+ assert(S);
+
+ if (const auto *Cond = dyn_cast<Expr>(S))
+ return Cond;
+
+ assert(isa<ObjCForCollectionStmt>(S) &&
+ "Only ObjCForCollectionStmt is known not to be a non-Expr terminator!");
+
+ // TODO: Return the collection.
+ return nullptr;
+}
+
+std::shared_ptr<PathDiagnosticPiece>
+TrackControlDependencyCondBRVisitor::VisitNode(const ExplodedNode *N,
+ BugReporterContext &BRC,
+ BugReport &BR) {
+ // We can only reason about control dependencies within the same stack frame.
+ if (Origin->getStackFrame() != N->getStackFrame())
+ return nullptr;
+
+ CFGBlock *NB = GetRelevantBlock(N);
+
+ // Skip if we already inspected this block.
+ if (!VisitedBlocks.insert(NB).second)
+ return nullptr;
+
+ CFGBlock *OriginB = GetRelevantBlock(Origin);
+ if (!OriginB || !NB)
+ return nullptr;
+
+ if (ControlDeps.isControlDependent(OriginB, NB))
+ if (const Expr *Condition = getTerminatorCondition(NB))
+ if (BR.addTrackedCondition(Condition))
+ bugreporter::trackExpressionValue(
+ N, Condition, BR, /*EnableNullFPSuppression=*/false);
+
+ return nullptr;
+}
+
//===----------------------------------------------------------------------===//
// Implementation of trackExpressionValue.
//===----------------------------------------------------------------------===//
@@ -1670,6 +1766,10 @@
ProgramStateRef LVState = LVNode->getState();
+ if (LVState->getAnalysisManager().getAnalyzerOptions().ShouldTrackConditions)
+ report.addVisitor(llvm::make_unique<TrackControlDependencyCondBRVisitor>(
+ InputNode));
+
// 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.
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -153,6 +153,9 @@
/// \sa removeInvalidation
llvm::SmallSet<InvalidationRecord, 4> Invalidations;
+ /// Conditions we're already tracking.
+ llvm::SmallPtrSet<const Expr *, 4> TrackedConditions;
+
private:
// Used internally by BugReporter.
Symbols &getInterestingSymbols();
@@ -349,6 +352,10 @@
visitor_iterator visitor_begin() { return Callbacks.begin(); }
visitor_iterator visitor_end() { return Callbacks.end(); }
+ bool addTrackedCondition(const Expr *Cond) {
+ return TrackedConditions.insert(Cond).second;
+ }
+
/// Profile to identify equivalent bug reports for error report coalescing.
/// Reports are uniqued to ensure that we do not emit multiple diagnostics
/// for each bug.
Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
===================================================================
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -291,6 +291,11 @@
"the analyzer's progress related to ctu.",
false)
+ANALYZER_OPTION(bool, ShouldTrackConditions, "track-conditions",
+ "Whether to track conditions that are a control dependency of "
+ "an already tracked variable.",
+ false)
+
//===----------------------------------------------------------------------===//
// Unsinged analyzer options.
//===----------------------------------------------------------------------===//
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
