[clang] [analyzer][NFC] Explain why operator new/delete should never be eval-called (PR #161370)
Balázs Benics via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 30 07:21:26 PDT 2025
https://github.com/balazs-benics-sonarsource updated https://github.com/llvm/llvm-project/pull/161370
>From 2d0a111ebf0262a4f814e1afefce63f996f6aaae Mon Sep 17 00:00:00 2001
From: Balazs Benics <balazs.benics at sonarsource.com>
Date: Tue, 30 Sep 2025 14:23:49 +0200
Subject: [PATCH 1/3] [analyzer][NFC] Explain why operator new/delete should
never be eval-called
Downstream, some change triggered an investigation if we could move a
checker callback from check::PostCall to eval::Call.
After a lengthy investigation that lead to ExprEngine::VisitCXXNewExpr
we realized that CXXNewExprs only trigger a PreCall and PostCall, but
never an EvalCall. It also had a fixme that maybe it should trigger it.
Remember, it called `defaultEvalCall` which either inlines or
conservatively evaluates aka. invalidates the call. But never propes the
checker evalCalls to see if any would step in.
After implementing the changes to trigger the eval call for the
checkers, I realized that it doesn't really make sense because we are
eval-calling user-provided functions, that we can't be really sure about
their semantics, thus there is no generic way to properly implement the
eval call callback.
This touches on an important point. It only ever makes sense to eval
call functions that has a clear spec. such as standard functions, as
implementing the callback would prevent the inlining of that function,
risking regressing analysis quality if the implemented model is not
complete/correct enough.
As a conclusion, I opted for not exposing the eval call event to
checkers, in other words, keep everything as-is, but document my
journey.
CPP-6585
---
.../Checkers/AnalysisOrderChecker.cpp | 3 ++-
.../Checkers/CheckerDocumentation.cpp | 8 ++++++++
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 12 +++++++++++-
.../Analysis/cxxctr-evalcall-analysis-order.cpp | 17 +++++++++++++++++
4 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
index e64153d53bbd6..309e3d250de06 100644
--- a/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -129,7 +129,8 @@ class AnalysisOrderChecker
llvm::errs() << " {argno: " << Call.getNumArgs() << '}';
llvm::errs() << " [" << Call.getKindAsString() << ']';
llvm::errs() << '\n';
- return true;
+ // We can't return `true` from this callback without binding the return
+ // value. Let's just fallthrough here and return `false`.
}
return false;
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
index 392c7eeea234a..b08a81f2889fd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
@@ -262,6 +262,14 @@ class CheckerDocumentation
/// state. This callback allows a checker to provide domain specific knowledge
/// about the particular functions it knows about.
///
+ /// Note that to evaluate a call, the handler MUST bind the return value if
+ /// its a non-void function. Invalidate the arguments if necessary.
+ ///
+ /// Note that in general, user-provided functions should not be eval-called
+ /// because the checker can't predict the exact semantics/contract of the
+ /// callee, and by having the eval::Call callback, we also prevent it from
+ /// getting inlined, potentially regressing analysis quality.
+ ///
/// \returns true if the call has been successfully evaluated
/// and false otherwise. Note, that only one checker can evaluate a call. If
/// more than one checker claims that they can evaluate the same call the
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index dee34e3e9d6a5..7a9d097861f42 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -909,7 +909,13 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
ExplodedNodeSet DstPostCall;
StmtNodeBuilder CallBldr(DstPreCall, DstPostCall, *currBldrCtx);
for (ExplodedNode *I : DstPreCall) {
- // FIXME: Provide evalCall for checkers?
+ // Operator new calls (CXXNewExpr) are intentionally not eval-called,
+ // because it does not make sense to eval-call user-provided functions.
+ // 1) If the new operator can be inlined, then don't prevent it from
+ // inlining by having an eval-call of that operator.
+ // 2) If it can't be inlined, then the default conservative modeling
+ // is what we want anyway.
+ // So the best is to not allow eval-calling CXXNewExprs from checkers.
defaultEvalCall(CallBldr, I, *Call);
}
// If the call is inlined, DstPostCall will be empty and we bail out now.
@@ -1110,6 +1116,10 @@ void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
if (AMgr.getAnalyzerOptions().MayInlineCXXAllocator) {
StmtNodeBuilder Bldr(DstPreCall, DstPostCall, *currBldrCtx);
for (ExplodedNode *I : DstPreCall) {
+ // Intentionally either inline or conservative eval-call the operator
+ // delete, but triggering an eval-call event for checkers.
+ // As detailed at handling CXXNewExprs, in short, because it does not
+ // really make sense to eval-call user-provided functions.
defaultEvalCall(Bldr, I, *Call);
}
} else {
diff --git a/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp b/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
index 0e1ec2f9de566..743c5ad0fa8cd 100644
--- a/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
+++ b/clang/test/Analysis/cxxctr-evalcall-analysis-order.cpp
@@ -18,16 +18,33 @@ void foo() {
C C0;
C C1(42);
C *C2 = new C{2, 3};
+ delete C2;
}
// CHECK: PreCall (C::C) [CXXConstructorCall]
// CHECK-NEXT: EvalCall (C::C) {argno: 0} [CXXConstructorCall]
// CHECK-NEXT: PostCall (C::C) [CXXConstructorCall]
+
// CHECK-NEXT: PreCall (C::C) [CXXConstructorCall]
// CHECK-NEXT: EvalCall (C::C) {argno: 1} [CXXConstructorCall]
// CHECK-NEXT: PostCall (C::C) [CXXConstructorCall]
+
// CHECK-NEXT: PreCall (operator new) [CXXAllocatorCall]
+// COMMENT: Operator new calls (CXXNewExpr) are intentionally not eval-called,
+// COMMENT: because it does not make sense to eval call user-provided functions.
+// COMMENT: 1) If the new operator can be inlined, then don't prevent it from
+// COMMENT: inlining by having an eval-call of that operator.
+// COMMENT: 2) If it can't be inlined, then the default conservative modeling
+// COMMENT: is what we anyways want anyway.
+// COMMENT: So the EvalCall event will not be triggered for operator new calls.
+// CHECK-NOT: EvalCall
// CHECK-NEXT: PostCall (operator new) [CXXAllocatorCall]
+
// CHECK-NEXT: PreCall (C::C) [CXXConstructorCall]
// CHECK-NEXT: EvalCall (C::C) {argno: 2} [CXXConstructorCall]
// CHECK-NEXT: PostCall (C::C) [CXXConstructorCall]
+
+// CHECK-NEXT: PreCall (operator delete) [CXXDeallocatorCall]
+// COMMENT: Same reasoning as for CXXNewExprs above.
+// CHECK-NOT: EvalCall
+// CHECK-NEXT: PostCall (operator delete) [CXXDeallocatorCall]
>From 57a083cdf2cc0f36841e88ff87971d317811128f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20Benics?=
<108414871+balazs-benics-sonarsource at users.noreply.github.com>
Date: Tue, 30 Sep 2025 15:57:55 +0200
Subject: [PATCH 2/3] Mention PreCall/PostCall as an alternative.
---
clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp | 1 +
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 1 +
2 files changed, 2 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
index b08a81f2889fd..c71623575ae97 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp
@@ -269,6 +269,7 @@ class CheckerDocumentation
/// because the checker can't predict the exact semantics/contract of the
/// callee, and by having the eval::Call callback, we also prevent it from
/// getting inlined, potentially regressing analysis quality.
+ /// Consider using check::PreCall or check::PostCall to allow inlining.
///
/// \returns true if the call has been successfully evaluated
/// and false otherwise. Note, that only one checker can evaluate a call. If
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 7a9d097861f42..738839356c91c 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -916,6 +916,7 @@ void ExprEngine::VisitCXXNewAllocatorCall(const CXXNewExpr *CNE,
// 2) If it can't be inlined, then the default conservative modeling
// is what we want anyway.
// So the best is to not allow eval-calling CXXNewExprs from checkers.
+ // Checkers can provide their pre/post-call callbacks if needed.
defaultEvalCall(CallBldr, I, *Call);
}
// If the call is inlined, DstPostCall will be empty and we bail out now.
>From 251e55d700cbf8ff4b5f9bf58fe9abc9be2932f1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20Benics?=
<108414871+balazs-benics-sonarsource at users.noreply.github.com>
Date: Tue, 30 Sep 2025 16:21:17 +0200
Subject: [PATCH 3/3] Add the missing `avoid` in comment
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Donát Nagy <donat.nagy at ericsson.com>
---
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 738839356c91c..75d7e265af0f3 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1118,7 +1118,7 @@ void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
StmtNodeBuilder Bldr(DstPreCall, DstPostCall, *currBldrCtx);
for (ExplodedNode *I : DstPreCall) {
// Intentionally either inline or conservative eval-call the operator
- // delete, but triggering an eval-call event for checkers.
+ // delete, but avoid triggering an eval-call event for checkers.
// As detailed at handling CXXNewExprs, in short, because it does not
// really make sense to eval-call user-provided functions.
defaultEvalCall(Bldr, I, *Call);
More information about the cfe-commits
mailing list