From 4e6f2ce82744322a35614532732281eacb2fe79a Mon Sep 17 00:00:00 2001 From: Ziqing Luo Date: Mon, 5 May 2025 14:27:48 -0700 Subject: [PATCH 1/4] [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 + class default_delete { + T dump(); + static T x; + }; + template > +#else + + struct default_delete {}; + template +#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 a(new X()); // previously leak falsely reported + return 0; +} From 148008d8e6058485821c784686ca0476faef6254 Mon Sep 17 00:00:00 2001 From: Ziqing Luo Date: Tue, 6 May 2025 16:06:49 -0700 Subject: [PATCH 2/4] 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(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 +#else + // Class with methods and static members is still empty: template class default_delete { T dump(); static T x; }; template > -#else - - struct default_delete {}; - template #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 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 a(new X()); + return 0; } From 603125c90de406c605970b35530ec3b4613bc40c Mon Sep 17 00:00:00 2001 From: Ziqing Luo Date: Tue, 6 May 2025 17:42:59 -0700 Subject: [PATCH 3/4] disale the test on windowns --- clang/test/Analysis/issue-137252.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/test/Analysis/issue-137252.cpp b/clang/test/Analysis/issue-137252.cpp index 6cc0af21f1f85..b0c71bfd5a251 100644 --- a/clang/test/Analysis/issue-137252.cpp +++ b/clang/test/Analysis/issue-137252.cpp @@ -1,6 +1,6 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus -verify %s // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus -verify %s -DEMPTY_CLASS - +// UNSUPPORTED: system-windows // expected-no-diagnostics // This test reproduces the issue that previously the static analyzer From 6cc591d1a4148a4064c8a552cfb3157e763a853c Mon Sep 17 00:00:00 2001 From: Ziqing Luo Date: Wed, 7 May 2025 11:17:01 -0700 Subject: [PATCH 4/4] Address comments --- clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp | 2 +- clang/test/Analysis/issue-137252.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp index 184569336b9e8..ff07402a29bba 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp @@ -717,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 = - dyn_cast_or_null(CE->getType()->getAsRecordDecl()); + cast(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 b0c71bfd5a251..6ca3e20ccbbca 100644 --- a/clang/test/Analysis/issue-137252.cpp +++ b/clang/test/Analysis/issue-137252.cpp @@ -4,7 +4,7 @@ // expected-no-diagnostics // This test reproduces the issue that previously the static analyzer -// initialized an [[__no_unique_address__]] empty field to zero, +// initialized an [[no_unique_address]] empty field to zero, // over-writing a non-empty field with the same offset. namespace std {