[clang] dddeec4 - [analyzer] Avoid out-of-order node traversal on void return (#117863)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 27 05:27:37 PST 2024
Author: Arseniy Zaostrovnykh
Date: 2024-11-27T14:27:31+01:00
New Revision: dddeec4becabf71d4067080bcc2c09a9e67c3025
URL: https://github.com/llvm/llvm-project/commit/dddeec4becabf71d4067080bcc2c09a9e67c3025
DIFF: https://github.com/llvm/llvm-project/commit/dddeec4becabf71d4067080bcc2c09a9e67c3025.diff
LOG: [analyzer] Avoid out-of-order node traversal on void return (#117863)
The motivating example: https://compiler-explorer.com/z/WjsxYfs43
```C++
#include <stdlib.h>
void inf_loop_break_callee() {
void* data = malloc(10);
while (1) {
(void)data; // line 3
break; // -> execution continues on line 3 ?!!
}
}
```
To correct the flow steps in this example (see the fixed version in the
added test case) I changed two things in the engine:
- Make `processCallExit` create a new StmtPoint only for return
statements. If the last non-jump statement is not a return statement,
e.g. `(void)data;`, it is no longer inserted in the exploded graph after
the function exit.
- Skip the purge program points. In the example above, purge
points are still inserted after the `break;` executes. Now, when the bug
reporter is looking for the next statement executed after the function
execution is finished, it will ignore the purge program points, so it
won't confusingly pick the `(void)data;` statement.
CPP-5778
Added:
clang/test/Analysis/void-call-exit-modelling.c
Modified:
clang/lib/StaticAnalyzer/Core/BugReporter.cpp
clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
clang/test/Analysis/copy-elision.cpp
clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index b67e6cd86c3d60..2904eab0097dc8 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2416,6 +2416,28 @@ PathSensitiveBugReport::getRanges() const {
return Ranges;
}
+static bool exitingDestructor(const ExplodedNode *N) {
+ // Need to loop here, as some times the Error node is already outside of the
+ // destructor context, and the previous node is an edge that is also outside.
+ while (N && !N->getLocation().getAs<StmtPoint>()) {
+ N = N->getFirstPred();
+ }
+ return N && isa<CXXDestructorDecl>(N->getLocationContext()->getDecl());
+}
+
+static const Stmt *
+findReasonableStmtCloseToFunctionExit(const ExplodedNode *N) {
+ if (exitingDestructor(N)) {
+ // If we are exiting a destructor call, it is more useful to point to
+ // the next stmt which is usually the temporary declaration.
+ if (const Stmt *S = N->getNextStmtForDiagnostics())
+ return S;
+ // If next stmt is not found, it is likely the end of a top-level
+ // function analysis. find the last execution statement then.
+ }
+ return N->getPreviousStmtForDiagnostics();
+}
+
PathDiagnosticLocation
PathSensitiveBugReport::getLocation() const {
assert(ErrorNode && "Cannot create a location with a null node.");
@@ -2433,15 +2455,7 @@ PathSensitiveBugReport::getLocation() const {
if (const ReturnStmt *RS = FE->getStmt())
return PathDiagnosticLocation::createBegin(RS, SM, LC);
- // If we are exiting a destructor call, it is more useful to point to the
- // next stmt which is usually the temporary declaration.
- // For non-destructor and non-top-level calls, the next stmt will still
- // refer to the last executed stmt of the body.
- S = ErrorNode->getNextStmtForDiagnostics();
- // If next stmt is not found, it is likely the end of a top-level function
- // analysis. find the last execution statement then.
- if (!S)
- S = ErrorNode->getPreviousStmtForDiagnostics();
+ S = findReasonableStmtCloseToFunctionExit(ErrorNode);
}
if (!S)
S = ErrorNode->getNextStmtForDiagnostics();
diff --git a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
index 11cef00ada330b..1e0cc2eea9ed85 100644
--- a/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
@@ -349,6 +349,8 @@ const Stmt *ExplodedNode::getStmtForDiagnostics() const {
const Stmt *ExplodedNode::getNextStmtForDiagnostics() const {
for (const ExplodedNode *N = getFirstSucc(); N; N = N->getFirstSucc()) {
+ if (N->getLocation().isPurgeKind())
+ continue;
if (const Stmt *S = N->getStmtForDiagnostics()) {
// Check if the statement is '?' or '&&'/'||'. These are "merges",
// not actual statement points.
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 2ca24d0c5ab22b..02facf786830d2 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -353,14 +353,19 @@ void ExprEngine::processCallExit(ExplodedNode *CEBNode) {
ExplodedNodeSet CleanedNodes;
if (LastSt && Blk && AMgr.options.AnalysisPurgeOpt != PurgeNone) {
static SimpleProgramPointTag retValBind("ExprEngine", "Bind Return Value");
- PostStmt Loc(LastSt, calleeCtx, &retValBind);
+ auto Loc = isa<ReturnStmt>(LastSt)
+ ? ProgramPoint{PostStmt(LastSt, calleeCtx, &retValBind)}
+ : ProgramPoint{EpsilonPoint(calleeCtx, /*Data1=*/nullptr,
+ /*Data2=*/nullptr, &retValBind)};
+ const CFGBlock *PrePurgeBlock =
+ isa<ReturnStmt>(LastSt) ? Blk : &CEBNode->getCFG().getExit();
bool isNew;
ExplodedNode *BindedRetNode = G.getNode(Loc, state, false, &isNew);
BindedRetNode->addPredecessor(CEBNode, G);
if (!isNew)
return;
- NodeBuilderContext Ctx(getCoreEngine(), Blk, BindedRetNode);
+ NodeBuilderContext Ctx(getCoreEngine(), PrePurgeBlock, BindedRetNode);
currBldrCtx = &Ctx;
// Here, we call the Symbol Reaper with 0 statement and callee location
// context, telling it to clean up everything in the callee's context
diff --git a/clang/test/Analysis/copy-elision.cpp b/clang/test/Analysis/copy-elision.cpp
index 423c4519f5bfc6..cd941854fc7f19 100644
--- a/clang/test/Analysis/copy-elision.cpp
+++ b/clang/test/Analysis/copy-elision.cpp
@@ -263,17 +263,17 @@ void testVariable() {
struct TestCtorInitializer {
ClassWithDestructor c;
- TestCtorInitializer(AddressVector<ClassWithDestructor> &v)
- : c(ClassWithDestructor(v)) {}
- // no-elide-warning at -1 {{Address of stack memory associated with temporary \
-object of type 'ClassWithDestructor' is still referred \
-to by the caller variable 'v' upon returning to the caller}}
+ TestCtorInitializer(AddressVector<ClassWithDestructor> &refParam)
+ : c(ClassWithDestructor(refParam)) {}
};
void testCtorInitializer() {
AddressVector<ClassWithDestructor> v;
{
TestCtorInitializer t(v);
+ // no-elide-warning at -1 {{Address of stack memory associated with temporary \
+object of type 'ClassWithDestructor' is still referred \
+to by the caller variable 'v' upon returning to the caller}}
// Check if the last destructor is an automatic destructor.
// A temporary destructor would have fired by now.
#if ELIDE
diff --git a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
index 53e72e7c5f4f7c..611e1d8255976c 100644
--- a/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
+++ b/clang/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
@@ -163,8 +163,8 @@ class UnguardedFieldThroughMethodTest {
Volume = 0;
break;
case A:
- Area = 0; // expected-warning {{1 uninitialized field}}
- break;
+ Area = 0;
+ break; // expected-warning {{1 uninitialized field}}
}
}
@@ -201,8 +201,8 @@ class UnguardedPublicFieldsTest {
Volume = 0;
break;
case A:
- Area = 0; // expected-warning {{1 uninitialized field}}
- break;
+ Area = 0;
+ break; // expected-warning {{1 uninitialized field}}
}
}
diff --git a/clang/test/Analysis/void-call-exit-modelling.c b/clang/test/Analysis/void-call-exit-modelling.c
new file mode 100644
index 00000000000000..6fc8cb54f00bcd
--- /dev/null
+++ b/clang/test/Analysis/void-call-exit-modelling.c
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -analyzer-output text -verify %s
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t size);
+
+void inf_loop_break_callee() {
+ void* data = malloc(10); // expected-note{{Memory is allocated}}
+ while (1) { // expected-note{{Loop condition is true}}
+ (void)data;
+ break; // No note that we jump to the line above from this break
+ } // expected-note at -1{{Execution jumps to the end of the function}}
+} // expected-warning{{Potential leak of memory pointed to by 'data'}}
+// expected-note at -1 {{Potential leak of memory pointed to by 'data'}}
+
+void inf_loop_break_caller() {
+ inf_loop_break_callee(); // expected-note{{Calling 'inf_loop_break_callee'}}
+}
+
+void inf_loop_break_top() {
+ void* data = malloc(10); // expected-note{{Memory is allocated}}
+ while (1) { // expected-note{{Loop condition is true}}
+ (void)data;
+ break; // No note that we jump to the line above from this break
+ } // expected-note at -1{{Execution jumps to the end of the function}}
+} // expected-warning{{Potential leak of memory pointed to by 'data'}}
+// expected-note at -1 {{Potential leak of memory pointed to by 'data'}}
More information about the cfe-commits
mailing list