[clang] [StaticAnalyzer] Make it a noop when initializing a field of empty record (PR #138594)
Ziqing Luo via cfe-commits
cfe-commits at lists.llvm.org
Tue May 6 16:10:32 PDT 2025
https://github.com/ziqingluo-90 updated https://github.com/llvm/llvm-project/pull/138594
>From 4e6f2ce82744322a35614532732281eacb2fe79a Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Mon, 5 May 2025 14:27:48 -0700
Subject: [PATCH 1/2] [StaticAnalyzer] Make it a noop when initializing a field
of empty record.
Previously, Static Analyzer initializes empty type fields with zeroes.
This can cause problems when those fields have no unique addresses.
For example, https://github.com/llvm/llvm-project/issues/137252.
rdar://146753089
---
.../lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 8 +++-
clang/test/Analysis/issue-137252.cpp | 45 +++++++++++++++++++
2 files changed, 52 insertions(+), 1 deletion(-)
create mode 100644 clang/test/Analysis/issue-137252.cpp
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 92ce3fa2225c8..219d7b4d2278c 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/ASTContext.h"
#include "clang/AST/AttrIterator.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/ParentMap.h"
@@ -700,6 +701,7 @@ void ExprEngine::handleConstructor(const Expr *E,
if (CE) {
// FIXME: Is it possible and/or useful to do this before PreStmt?
StmtNodeBuilder Bldr(DstPreVisit, PreInitialized, *currBldrCtx);
+ ASTContext &Ctx = LCtx->getAnalysisDeclContext()->getASTContext();
for (ExplodedNode *N : DstPreVisit) {
ProgramStateRef State = N->getState();
if (CE->requiresZeroInitialization()) {
@@ -715,7 +717,11 @@ void ExprEngine::handleConstructor(const Expr *E,
// actually make things worse. Placement new makes this tricky as well,
// since it's then possible to be initializing one part of a multi-
// dimensional array.
- State = State->bindDefaultZero(Target, LCtx);
+ const CXXRecordDecl *TargetHeldRecord =
+ Target.getType(Ctx)->getPointeeCXXRecordDecl();
+
+ if (!TargetHeldRecord || !TargetHeldRecord->isEmpty())
+ State = State->bindDefaultZero(Target, LCtx);
}
Bldr.generateNode(CE, N, State, /*tag=*/nullptr,
diff --git a/clang/test/Analysis/issue-137252.cpp b/clang/test/Analysis/issue-137252.cpp
new file mode 100644
index 0000000000000..8064e3f54d9fd
--- /dev/null
+++ b/clang/test/Analysis/issue-137252.cpp
@@ -0,0 +1,45 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus -verify %s -DEMPTY_CLASS
+
+// expected-no-diagnostics
+
+// This test reproduces the issue that previously the static analyzer
+// initialized an [[__no_unique_address__]] empty field to zero,
+// over-writing a non-empty field with the same offset.
+
+namespace std {
+#ifdef EMPTY_CLASS
+
+ template <typename T>
+ class default_delete {
+ T dump();
+ static T x;
+ };
+ template <class _Tp, class _Dp = default_delete<_Tp> >
+#else
+
+ struct default_delete {};
+ template <class _Tp, class _Dp = default_delete >
+#endif
+ class unique_ptr {
+ [[__no_unique_address__]] _Tp * __ptr_;
+ [[__no_unique_address__]] _Dp __deleter_;
+
+ public:
+ explicit unique_ptr(_Tp* __p) noexcept
+ : __ptr_(__p),
+ __deleter_() {}
+
+ ~unique_ptr() {
+ delete __ptr_;
+ }
+ };
+}
+
+struct X {};
+
+int main()
+{
+ std::unique_ptr<X> a(new X()); // previously leak falsely reported
+ return 0;
+}
>From 148008d8e6058485821c784686ca0476faef6254 Mon Sep 17 00:00:00 2001
From: Ziqing Luo <ziqing at udel.edu>
Date: Tue, 6 May 2025 16:06:49 -0700
Subject: [PATCH 2/2] address comments
---
.../lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 3 +--
clang/test/Analysis/issue-137252.cpp | 21 ++++++++++++-------
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index 219d7b4d2278c..184569336b9e8 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -701,7 +701,6 @@ void ExprEngine::handleConstructor(const Expr *E,
if (CE) {
// FIXME: Is it possible and/or useful to do this before PreStmt?
StmtNodeBuilder Bldr(DstPreVisit, PreInitialized, *currBldrCtx);
- ASTContext &Ctx = LCtx->getAnalysisDeclContext()->getASTContext();
for (ExplodedNode *N : DstPreVisit) {
ProgramStateRef State = N->getState();
if (CE->requiresZeroInitialization()) {
@@ -718,7 +717,7 @@ void ExprEngine::handleConstructor(const Expr *E,
// since it's then possible to be initializing one part of a multi-
// dimensional array.
const CXXRecordDecl *TargetHeldRecord =
- Target.getType(Ctx)->getPointeeCXXRecordDecl();
+ dyn_cast_or_null<CXXRecordDecl>(CE->getType()->getAsRecordDecl());
if (!TargetHeldRecord || !TargetHeldRecord->isEmpty())
State = State->bindDefaultZero(Target, LCtx);
diff --git a/clang/test/Analysis/issue-137252.cpp b/clang/test/Analysis/issue-137252.cpp
index 8064e3f54d9fd..6cc0af21f1f85 100644
--- a/clang/test/Analysis/issue-137252.cpp
+++ b/clang/test/Analysis/issue-137252.cpp
@@ -10,20 +10,20 @@
namespace std {
#ifdef EMPTY_CLASS
+ struct default_delete {};
+ template <class _Tp, class _Dp = default_delete >
+#else
+ // Class with methods and static members is still empty:
template <typename T>
class default_delete {
T dump();
static T x;
};
template <class _Tp, class _Dp = default_delete<_Tp> >
-#else
-
- struct default_delete {};
- template <class _Tp, class _Dp = default_delete >
#endif
class unique_ptr {
- [[__no_unique_address__]] _Tp * __ptr_;
- [[__no_unique_address__]] _Dp __deleter_;
+ [[no_unique_address]] _Tp * __ptr_;
+ [[no_unique_address]] _Dp __deleter_;
public:
explicit unique_ptr(_Tp* __p) noexcept
@@ -40,6 +40,11 @@ struct X {};
int main()
{
- std::unique_ptr<X> a(new X()); // previously leak falsely reported
- return 0;
+ // Previously a leak falsely reported here. It was because the
+ // Static Analyzer engine simulated the initialization of
+ // `__deleter__` incorrectly. The engine assigned zero to
+ // `__deleter__`--an empty record sharing offset with `__ptr__`.
+ // The assignment over wrote `__ptr__`.
+ std::unique_ptr<X> a(new X());
+ return 0;
}
More information about the cfe-commits
mailing list