Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, Charusso,
dcoughlin, rnkovacs, TWeaver.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, whisperity.
https://bugs.llvm.org/show_bug.cgi?id=43102
In today's edition of "Is this any better now that it isn't crashing?", I'd
like to show you a very interesting test case with loop widening.
Looking at the included test case, it's immediately obvious that this is not
only a false positive, but also a very bad bug report in general. We can see
how the analyzer mistakenly invalidated `b`, instead of its pointee, resulting
in it reporting a null pointer dereference error. Not only that, the point at
which this change of value is noted at is at the loop, rather then at the
method call.
It turns out that `FindLastStoreVisitor` works correctly, rather the supplied
explodedgraph is faulty, because `BlockEdge` really is the `ProgramPoint` where
this happens.
F9855739: image.png <https://reviews.llvm.org/F9855739>
So it's fair to say that this needs improving on multiple fronts. In any case,
at least the crash is gone.
Full ExplodedGraph: F9855743: loop.dot <https://reviews.llvm.org/F9855743>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D66716
Files:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
clang/test/Analysis/loop-widening.cpp
Index: clang/test/Analysis/loop-widening.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-config widen-loops=true \
+// RUN: -analyzer-config track-conditions=false \
+// RUN: -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+ void m_fn1();
+};
+bool g;
+void fn1() {
+ A a;
+ A *b = &a;
+
+ for (;;) { // expected-note{{Loop condition is true. Entering loop body}}
+ // expected-note@-1{{Loop condition is true. Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction
+
+ g = !b; // expected-note{{Assuming 'b' is null}}
+ b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+ // expected-note@-1{{Called C++ object pointer is null}}
+ }
+}
+} // end of namespace pr43102
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -761,14 +761,18 @@
return PathDiagnosticLocation(
CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
} else if (Optional<BlockEntrance> BE = P.getAs<BlockEntrance>()) {
- CFGElement BlockFront = BE->getBlock()->front();
- if (auto StmtElt = BlockFront.getAs<CFGStmt>()) {
- return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
- } else if (auto NewAllocElt = BlockFront.getAs<CFGNewAllocator>()) {
- return PathDiagnosticLocation(
- NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+ if (Optional<CFGElement> BlockFront = BE->getFirstElement()) {
+ if (auto StmtElt = BlockFront->getAs<CFGStmt>()) {
+ return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+ } else if (auto NewAllocElt = BlockFront->getAs<CFGNewAllocator>()) {
+ return PathDiagnosticLocation(
+ NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+ }
+ llvm_unreachable("Unexpected CFG element at front of block");
}
- llvm_unreachable("Unexpected CFG element at front of block");
+
+ return PathDiagnosticLocation(
+ BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
} else if (Optional<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
return PathDiagnosticLocation(FE->getStmt(), SMng,
FE->getLocationContext());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1441,6 +1441,7 @@
if (!StoreSite)
return nullptr;
+
Satisfied = true;
// If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@
if (ControlDeps.isControlDependent(OriginB, NB)) {
// We don't really want to explain for range loops. Evidence suggests that
// the only thing that leads to is the addition of calls to operator!=.
- if (isa<CXXForRangeStmt>(NB->getTerminator()))
+ if (llvm::isa_and_nonnull<CXXForRangeStmt>(NB->getTerminatorStmt()))
return nullptr;
if (const Expr *Condition = NB->getLastCondition()) {
Index: clang/test/Analysis/loop-widening.cpp
===================================================================
--- /dev/null
+++ clang/test/Analysis/loop-widening.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_analyze_cc1 -verify %s \
+// RUN: -analyzer-checker=core \
+// RUN: -analyzer-config widen-loops=true \
+// RUN: -analyzer-config track-conditions=false \
+// RUN: -analyzer-max-loop 2 -analyzer-output=text
+
+namespace pr43102 {
+class A {
+public:
+ void m_fn1();
+};
+bool g;
+void fn1() {
+ A a;
+ A *b = &a;
+
+ for (;;) { // expected-note{{Loop condition is true. Entering loop body}}
+ // expected-note@-1{{Loop condition is true. Entering loop body}}
+ // expected-note@-2{{Value assigned to 'b'}}
+ // no crash during bug report construction
+
+ g = !b; // expected-note{{Assuming 'b' is null}}
+ b->m_fn1(); // expected-warning{{Called C++ object pointer is null}}
+ // expected-note@-1{{Called C++ object pointer is null}}
+ }
+}
+} // end of namespace pr43102
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -761,14 +761,18 @@
return PathDiagnosticLocation(
CEB->getLocationContext()->getDecl()->getSourceRange().getEnd(), SMng);
} else if (Optional<BlockEntrance> BE = P.getAs<BlockEntrance>()) {
- CFGElement BlockFront = BE->getBlock()->front();
- if (auto StmtElt = BlockFront.getAs<CFGStmt>()) {
- return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
- } else if (auto NewAllocElt = BlockFront.getAs<CFGNewAllocator>()) {
- return PathDiagnosticLocation(
- NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+ if (Optional<CFGElement> BlockFront = BE->getFirstElement()) {
+ if (auto StmtElt = BlockFront->getAs<CFGStmt>()) {
+ return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
+ } else if (auto NewAllocElt = BlockFront->getAs<CFGNewAllocator>()) {
+ return PathDiagnosticLocation(
+ NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
+ }
+ llvm_unreachable("Unexpected CFG element at front of block");
}
- llvm_unreachable("Unexpected CFG element at front of block");
+
+ return PathDiagnosticLocation(
+ BE->getBlock()->getTerminatorStmt()->getBeginLoc(), SMng);
} else if (Optional<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
return PathDiagnosticLocation(FE->getStmt(), SMng,
FE->getLocationContext());
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1441,6 +1441,7 @@
if (!StoreSite)
return nullptr;
+
Satisfied = true;
// If we have an expression that provided the value, try to track where it
@@ -1802,7 +1803,7 @@
if (ControlDeps.isControlDependent(OriginB, NB)) {
// We don't really want to explain for range loops. Evidence suggests that
// the only thing that leads to is the addition of calls to operator!=.
- if (isa<CXXForRangeStmt>(NB->getTerminator()))
+ if (llvm::isa_and_nonnull<CXXForRangeStmt>(NB->getTerminatorStmt()))
return nullptr;
if (const Expr *Condition = NB->getLastCondition()) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
