[clang] [analyzer] Fix FP for cplusplus.placement new #149240 (PR #150161)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 29 14:02:53 PDT 2025
https://github.com/Aethezz updated https://github.com/llvm/llvm-project/pull/150161
>From a018c87c6356326cb04bd0a98ff5842e07403f9c Mon Sep 17 00:00:00 2001
From: Aethezz <64500703+Aethezz at users.noreply.github.com>
Date: Tue, 22 Jul 2025 23:19:11 -0400
Subject: [PATCH 1/4] fix false positive, warning is now only triggered when
the place size is strictly less than target size. Add test cases for when
place size == or > or < than target size.
---
.../Checkers/CheckPlacementNew.cpp | 28 ++++---------------
clang/test/SemaCXX/placement-new-bounds.cpp | 25 +++++++++++++++++
2 files changed, 30 insertions(+), 23 deletions(-)
create mode 100644 clang/test/SemaCXX/placement-new-bounds.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index 839c8bcd90210..4c4ca15efa4c9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -111,32 +111,14 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
if (!SizeOfPlaceCI)
return true;
- if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue()) ||
- (IsArrayTypeAllocated &&
- SizeOfPlaceCI->getValue() >= SizeOfTargetCI->getValue())) {
+ if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue())) {
if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
std::string Msg;
// TODO: use clang constant
- if (IsArrayTypeAllocated &&
- SizeOfPlaceCI->getValue() > SizeOfTargetCI->getValue())
- Msg = std::string(llvm::formatv(
- "{0} bytes is possibly not enough for array allocation which "
- "requires {1} bytes. Current overhead requires the size of {2} "
- "bytes",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue(),
- *SizeOfPlaceCI->getValue() - SizeOfTargetCI->getValue()));
- else if (IsArrayTypeAllocated &&
- SizeOfPlaceCI->getValue() == SizeOfTargetCI->getValue())
- Msg = std::string(llvm::formatv(
- "Storage provided to placement new is only {0} bytes, "
- "whereas the allocated array type requires more space for "
- "internal needs",
- SizeOfPlaceCI->getValue()));
- else
- Msg = std::string(llvm::formatv(
- "Storage provided to placement new is only {0} bytes, "
- "whereas the allocated type requires {1} bytes",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+ Msg = std::string(llvm::formatv(
+ "Storage provided to placement new is only {0} bytes, "
+ "whereas the allocated type requires {1} bytes",
+ SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
diff --git a/clang/test/SemaCXX/placement-new-bounds.cpp b/clang/test/SemaCXX/placement-new-bounds.cpp
new file mode 100644
index 0000000000000..49a7352922140
--- /dev/null
+++ b/clang/test/SemaCXX/placement-new-bounds.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang -fsyntax-only -Xclang -analyze -Xclang -analyzer-checker=cplusplus.PlacementNew -Xclang -verify -std=c++17 %s
+
+#include <new>
+
+void test_exact_size() {
+ void *buf = ::operator new(sizeof(int)*2);
+ int *placement_int = new (buf) int[2]; // no-warning
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
+
+void test_undersize() {
+ void *buf = ::operator new(sizeof(int)*1);
+ // Remember the exact text including the checker name, or use regex for robustness.
+ int *placement_int = new (buf) int[2]; // expected-warning {{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes [cplusplus.PlacementNew]}}
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
+
+void test_oversize() {
+ void *buf = ::operator new(sizeof(int)*4);
+ int *placement_int = new (buf) int[2]; // no-warning
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
\ No newline at end of file
>From d3e79946cf09a8c87e152b836b0b4f324c6cdd42 Mon Sep 17 00:00:00 2001
From: Aethezz <ellisonlao999 at gmail.com>
Date: Wed, 23 Jul 2025 12:27:27 -0400
Subject: [PATCH 2/4] Fixed formatting issues and removed expected warnings
from other test cases
---
clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp | 8 ++++----
clang/test/Analysis/placement-new.cpp | 8 ++++----
clang/test/SemaCXX/placement-new-bounds.cpp | 1 -
3 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index 4c4ca15efa4c9..2a57fa2e8ff4a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -115,10 +115,10 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
std::string Msg;
// TODO: use clang constant
- Msg = std::string(llvm::formatv(
- "Storage provided to placement new is only {0} bytes, "
- "whereas the allocated type requires {1} bytes",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+ Msg = std::string(
+ llvm::formatv("Storage provided to placement new is only {0} bytes, "
+ "whereas the allocated type requires {1} bytes",
+ SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
diff --git a/clang/test/Analysis/placement-new.cpp b/clang/test/Analysis/placement-new.cpp
index 766b11cf84c5f..82d1d87c59792 100644
--- a/clang/test/Analysis/placement-new.cpp
+++ b/clang/test/Analysis/placement-new.cpp
@@ -168,8 +168,8 @@ void f1() {
// bad (not enough space).
const unsigned N = 32;
- alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
- ::new (buffer1) S[N]; // expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+ alignas(S) unsigned char buffer1[sizeof(S) * N];
+ ::new (buffer1) S[N];
}
void f2() {
@@ -179,8 +179,8 @@ void f2() {
// maybe ok but we need to warn.
const unsigned N = 32;
- alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
- ::new (buffer2) S[N]; // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+ alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)];
+ ::new (buffer2) S[N];
}
} // namespace testArrayTypesAllocation
diff --git a/clang/test/SemaCXX/placement-new-bounds.cpp b/clang/test/SemaCXX/placement-new-bounds.cpp
index 49a7352922140..89bfb1292fc3e 100644
--- a/clang/test/SemaCXX/placement-new-bounds.cpp
+++ b/clang/test/SemaCXX/placement-new-bounds.cpp
@@ -11,7 +11,6 @@ void test_exact_size() {
void test_undersize() {
void *buf = ::operator new(sizeof(int)*1);
- // Remember the exact text including the checker name, or use regex for robustness.
int *placement_int = new (buf) int[2]; // expected-warning {{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes [cplusplus.PlacementNew]}}
placement_int[0] = 42;
::operator delete(buf);
>From b93bd2cfdf3b8299c952e60a64e0c378e6f81b47 Mon Sep 17 00:00:00 2001
From: Aethezz <ellisonlao999 at gmail.com>
Date: Fri, 25 Jul 2025 14:32:48 -0400
Subject: [PATCH 3/4] removed the redundant test file, update comments in
placement-new.cpp test file to explain the removal of checker warning the
rare case of allocating extra memory, and removed std::string explicit
conversion operator
---
.../Checkers/CheckPlacementNew.cpp | 7 +++---
clang/test/Analysis/placement-new.cpp | 6 +++--
clang/test/SemaCXX/placement-new-bounds.cpp | 24 -------------------
3 files changed, 7 insertions(+), 30 deletions(-)
delete mode 100644 clang/test/SemaCXX/placement-new-bounds.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index 2a57fa2e8ff4a..a4ea359758f14 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -113,12 +113,11 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue())) {
if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
- std::string Msg;
- // TODO: use clang constant
- Msg = std::string(
+ std::string Msg =
llvm::formatv("Storage provided to placement new is only {0} bytes, "
"whereas the allocated type requires {1} bytes",
- SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+ SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue());
+ // TODO: use clang constants
auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
diff --git a/clang/test/Analysis/placement-new.cpp b/clang/test/Analysis/placement-new.cpp
index 82d1d87c59792..db3717286bf8f 100644
--- a/clang/test/Analysis/placement-new.cpp
+++ b/clang/test/Analysis/placement-new.cpp
@@ -166,7 +166,8 @@ void f1() {
short a;
};
- // bad (not enough space).
+ // on some systems, placement array new may allocate more memory than the nominal size of the array
+ // in such cases, test code could be problematic, but the checker doesn't warn here because this behavior is expected to be rare
const unsigned N = 32;
alignas(S) unsigned char buffer1[sizeof(S) * N];
::new (buffer1) S[N];
@@ -177,7 +178,8 @@ void f2() {
short a;
};
- // maybe ok but we need to warn.
+ // on some systems, placement array new may allocate more memory than the nominal size of the array
+ // in such cases, test code could be problematic, but the checker doesn't warn here because this behavior is expected to be rare
const unsigned N = 32;
alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)];
::new (buffer2) S[N];
diff --git a/clang/test/SemaCXX/placement-new-bounds.cpp b/clang/test/SemaCXX/placement-new-bounds.cpp
deleted file mode 100644
index 89bfb1292fc3e..0000000000000
--- a/clang/test/SemaCXX/placement-new-bounds.cpp
+++ /dev/null
@@ -1,24 +0,0 @@
-// RUN: %clang -fsyntax-only -Xclang -analyze -Xclang -analyzer-checker=cplusplus.PlacementNew -Xclang -verify -std=c++17 %s
-
-#include <new>
-
-void test_exact_size() {
- void *buf = ::operator new(sizeof(int)*2);
- int *placement_int = new (buf) int[2]; // no-warning
- placement_int[0] = 42;
- ::operator delete(buf);
-}
-
-void test_undersize() {
- void *buf = ::operator new(sizeof(int)*1);
- int *placement_int = new (buf) int[2]; // expected-warning {{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes [cplusplus.PlacementNew]}}
- placement_int[0] = 42;
- ::operator delete(buf);
-}
-
-void test_oversize() {
- void *buf = ::operator new(sizeof(int)*4);
- int *placement_int = new (buf) int[2]; // no-warning
- placement_int[0] = 42;
- ::operator delete(buf);
-}
\ No newline at end of file
>From fd6db232a53596d043f193694a2c9a78d41b74b2 Mon Sep 17 00:00:00 2001
From: Aethezz <ellisonlao999 at gmail.com>
Date: Tue, 29 Jul 2025 17:02:38 -0400
Subject: [PATCH 4/4] Updated comments in test, added MSVC related links, and
dropped the dangling comment
---
.../Checkers/CheckPlacementNew.cpp | 1 -
clang/test/Analysis/placement-new.cpp | 30 +++++++++++++++----
2 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index a4ea359758f14..a227ca0cb70bb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -117,7 +117,6 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
llvm::formatv("Storage provided to placement new is only {0} bytes, "
"whereas the allocated type requires {1} bytes",
SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue());
- // TODO: use clang constants
auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
diff --git a/clang/test/Analysis/placement-new.cpp b/clang/test/Analysis/placement-new.cpp
index db3717286bf8f..50bbde29cfd59 100644
--- a/clang/test/Analysis/placement-new.cpp
+++ b/clang/test/Analysis/placement-new.cpp
@@ -166,11 +166,29 @@ void f1() {
short a;
};
- // on some systems, placement array new may allocate more memory than the nominal size of the array
- // in such cases, test code could be problematic, but the checker doesn't warn here because this behavior is expected to be rare
+ // On some systems, (notably before MSVC 16.7), a non-allocating placement
+ // array new could allocate more memory than the nominal size of the array.
+
+ // Since CWG 2382 (implemented in MSVC 16.7), overhead was disallowed for non-allocating placement new.
+ // See:
+ // https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170
+ // https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1969r0.html#2382
+
+ // However, as of 17.1, there is a regression when the type comes from a template
+ // parameter where MSVC reintroduces overhead.
+ // See:
+ // https://developercommunity.visualstudio.com/t/10777485
+ // https://godbolt.org/z/E1z1Tsfvj
+
+ // The checker doesn't warn here because this behavior only affects older
+ // MSVC versions (<16.7) or certain specific versions (17.1).
+ // Suppressing warnings avoids false positives on standard-compliant compilers
+ // and modern MSVC versions, but users of affected MSVC versions should be
+ // aware of potential buffer size issues.
+
const unsigned N = 32;
alignas(S) unsigned char buffer1[sizeof(S) * N];
- ::new (buffer1) S[N];
+ ::new (buffer1) S[N]; // no-warning: See comments above
}
void f2() {
@@ -178,11 +196,11 @@ void f2() {
short a;
};
- // on some systems, placement array new may allocate more memory than the nominal size of the array
- // in such cases, test code could be problematic, but the checker doesn't warn here because this behavior is expected to be rare
+ // On some systems, placement array new could allocate more memory than the nominal size of the array.
+ // See the comment at f1() above for more details.
const unsigned N = 32;
alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)];
- ::new (buffer2) S[N];
+ ::new (buffer2) S[N]; // no-warning: See comments above
}
} // namespace testArrayTypesAllocation
More information about the cfe-commits
mailing list