[clang] e3e9082 - [analyzer] Fix for incorrect handling of 0 length non-POD array construction
via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 25 03:42:16 PDT 2022
Author: isuckatcs
Date: 2022-08-25T12:42:02+02:00
New Revision: e3e9082b013ba948e3df6d4a2536df9d04656e76
URL: https://github.com/llvm/llvm-project/commit/e3e9082b013ba948e3df6d4a2536df9d04656e76
DIFF: https://github.com/llvm/llvm-project/commit/e3e9082b013ba948e3df6d4a2536df9d04656e76.diff
LOG: [analyzer] Fix for incorrect handling of 0 length non-POD array construction
Prior to this patch when the analyzer encountered a non-POD 0 length array,
it still invoked the constructor for 1 element, which lead to false positives.
This patch makes sure that we no longer construct any elements when we see a
0 length array.
Differential Revision: https://reviews.llvm.org/D131501
Added:
clang/test/Analysis/flexible-array-member.cpp
clang/test/Analysis/zero-size-non-pod-array.cpp
Modified:
clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
clang/test/Analysis/array-init-loop.cpp
clang/test/Analysis/dtor-array.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index eacacc4e1d611..ae878ecbcc34a 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -606,6 +606,27 @@ void ExprEngine::handleConstructor(const Expr *E,
unsigned Idx = 0;
if (CE->getType()->isArrayType() || AILE) {
+
+ auto isZeroSizeArray = [&] {
+ uint64_t Size = 1;
+
+ if (const auto *CAT = dyn_cast<ConstantArrayType>(CE->getType()))
+ Size = getContext().getConstantArrayElementCount(CAT);
+ else if (AILE)
+ Size = getContext().getArrayInitLoopExprElementCount(AILE);
+
+ return Size == 0;
+ };
+
+ // No element construction will happen in a 0 size array.
+ if (isZeroSizeArray()) {
+ StmtNodeBuilder Bldr(Pred, destNodes, *currBldrCtx);
+ static SimpleProgramPointTag T{"ExprEngine",
+ "Skipping 0 size array construction"};
+ Bldr.generateNode(CE, Pred, State, &T);
+ return;
+ }
+
Idx = getIndexOfElementToConstruct(State, CE, LCtx).value_or(0u);
State = setIndexOfElementToConstruct(State, CE, LCtx, Idx + 1);
}
@@ -1165,6 +1186,16 @@ void ExprEngine::VisitLambdaExpr(const LambdaExpr *LE, ExplodedNode *Pred,
assert(InitExpr && "Capture missing initialization expression");
+ // Capturing a 0 length array is a no-op, so we ignore it to get a more
+ // accurate analysis. If it's not ignored, it would set the default
+ // binding of the lambda to 'Unknown', which can lead to falsely detecting
+ // 'Uninitialized' values as 'Unknown' and not reporting a warning.
+ const auto FTy = FieldForCapture->getType();
+ if (FTy->isConstantArrayType() &&
+ getContext().getConstantArrayElementCount(
+ getContext().getAsConstantArrayType(FTy)) == 0)
+ continue;
+
// With C++17 copy elision the InitExpr can be anything, so instead of
// pattern matching all cases, we simple check if the current field is
// under construction or not, regardless what it's InitExpr is.
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index eca012b6015fa..4670ae0fb248e 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2594,6 +2594,12 @@ RegionStoreManager::tryBindSmallStruct(RegionBindingsConstRef B,
return None;
QualType Ty = FD->getType();
+
+ // Zero length arrays are basically no-ops, so we also ignore them here.
+ if (Ty->isConstantArrayType() &&
+ Ctx.getConstantArrayElementCount(Ctx.getAsConstantArrayType(Ty)) == 0)
+ continue;
+
if (!(Ty->isScalarType() || Ty->isReferenceType()))
return None;
diff --git a/clang/test/Analysis/array-init-loop.cpp b/clang/test/Analysis/array-init-loop.cpp
index ca7c832e8ab13..4ab4489fc882f 100644
--- a/clang/test/Analysis/array-init-loop.cpp
+++ b/clang/test/Analysis/array-init-loop.cpp
@@ -306,3 +306,27 @@ void structured_binding_multi() {
clang_analyzer_eval(b[0].i == 3); // expected-warning{{TRUE}}
clang_analyzer_eval(b[1].i == 4); // expected-warning{{TRUE}}
}
+
+// This snippet used to crash
+namespace crash {
+
+struct S
+{
+ int x;
+ S() { x = 1; }
+};
+
+void no_crash() {
+ S arr[0];
+ int n = 1;
+
+ auto l = [arr, n] { return n; };
+
+ int x = l();
+ clang_analyzer_eval(x == 1); // expected-warning{{TRUE}}
+
+ // FIXME: This should be 'Undefined'.
+ clang_analyzer_eval(arr[0].x); // expected-warning{{UNKNOWN}}
+}
+
+} // namespace crash
diff --git a/clang/test/Analysis/dtor-array.cpp b/clang/test/Analysis/dtor-array.cpp
index da1c80e261948..ab0a939232173 100644
--- a/clang/test/Analysis/dtor-array.cpp
+++ b/clang/test/Analysis/dtor-array.cpp
@@ -258,8 +258,7 @@ void zeroLength(){
auto *arr4 = new InlineDtor[2][2][0];
delete[] arr4;
- // FIXME: Should be TRUE once the constructors are handled properly.
- clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}} expected-warning {{FALSE}}
+ clang_analyzer_eval(InlineDtor::dtorCalled == 0); // expected-warning {{TRUE}}
}
diff --git a/clang/test/Analysis/flexible-array-member.cpp b/clang/test/Analysis/flexible-array-member.cpp
new file mode 100644
index 0000000000000..10ca64bc43805
--- /dev/null
+++ b/clang/test/Analysis/flexible-array-member.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_eval(bool);
+
+struct S
+{
+ static int c;
+ static int d;
+ int x;
+ S() { x = c++; }
+ ~S() { d++; }
+};
+
+int S::c = 0;
+int S::d = 0;
+
+struct Flex
+{
+ int length;
+ S contents[0];
+};
+
+void flexibleArrayMember()
+{
+ S::c = 0;
+ S::d = 0;
+
+ const int size = 4;
+
+ Flex *arr =
+ (Flex *)::operator new(__builtin_offsetof(Flex, contents) + sizeof(S) * size);
+
+ clang_analyzer_eval(S::c == 0); // expected-warning{{TRUE}}
+
+ new (&arr->contents[0]) S;
+ new (&arr->contents[1]) S;
+ new (&arr->contents[2]) S;
+ new (&arr->contents[3]) S;
+
+ clang_analyzer_eval(S::c == size); // expected-warning{{TRUE}}
+
+ clang_analyzer_eval(arr->contents[0].x == 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr->contents[1].x == 1); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr->contents[2].x == 2); // expected-warning{{TRUE}}
+ clang_analyzer_eval(arr->contents[3].x == 3); // expected-warning{{TRUE}}
+
+ arr->contents[0].~S();
+ arr->contents[1].~S();
+ arr->contents[2].~S();
+ arr->contents[3].~S();
+
+ ::operator delete(arr);
+
+ clang_analyzer_eval(S::d == size); // expected-warning{{TRUE}}
+}
diff --git a/clang/test/Analysis/zero-size-non-pod-array.cpp b/clang/test/Analysis/zero-size-non-pod-array.cpp
new file mode 100644
index 0000000000000..a2117bc80eac8
--- /dev/null
+++ b/clang/test/Analysis/zero-size-non-pod-array.cpp
@@ -0,0 +1,189 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -std=c++17 -verify %s
+
+void clang_analyzer_eval(bool);
+
+struct S{
+ static int CtorInvocationCount;
+ static int DtorInvocationCount;
+
+ S(){CtorInvocationCount++;}
+ ~S(){DtorInvocationCount++;}
+};
+
+int S::CtorInvocationCount = 0;
+int S::DtorInvocationCount = 0;
+
+void zeroSizeArrayStack() {
+ S::CtorInvocationCount = 0;
+
+ S arr[0];
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeMultidimensionalArrayStack() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ {
+ S arr[2][0];
+ S arr2[0][2];
+
+ S arr3[0][2][2];
+ S arr4[2][2][0];
+ S arr5[2][0][2];
+ }
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayStackInLambda() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ []{
+ S arr[0];
+ }();
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayHeap() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ auto *arr = new S[0];
+ delete[] arr;
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeMultidimensionalArrayHeap() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ auto *arr = new S[2][0];
+ delete[] arr;
+
+ auto *arr2 = new S[0][2];
+ delete[] arr2;
+
+ auto *arr3 = new S[0][2][2];
+ delete[] arr3;
+
+ auto *arr4 = new S[2][2][0];
+ delete[] arr4;
+
+ auto *arr5 = new S[2][0][2];
+ delete[] arr5;
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+#if __cplusplus >= 201703L
+
+void zeroSizeArrayBinding() {
+ S::CtorInvocationCount = 0;
+
+ S arr[0];
+
+ // Note: This is an error in gcc but a warning in clang.
+ // In MSVC the declaration of 'S arr[0]' is already an error
+ // and it doesn't recognize this syntax as a structured binding.
+ auto [] = arr; //expected-warning{{ISO C++17 does not allow a decomposition group to be empty}}
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+#endif
+
+void zeroSizeArrayLambdaCapture() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ S arr[0];
+
+ auto l = [arr]{};
+ [arr]{}();
+
+ //FIXME: These should be TRUE. We should avoid calling the destructor
+ // of the temporary that is materialized as the lambda.
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}} expected-warning{{FALSE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}} expected-warning{{FALSE}}
+}
+
+// FIXME: Report a warning if the standard is at least C++17.
+#if __cplusplus < 201703L
+void zeroSizeArrayLambdaCaptureUndefined1() {
+ S arr[0];
+ int n;
+
+ auto l = [arr, n]{
+ int x = n; //expected-warning{{Assigned value is garbage or undefined}}
+ (void) x;
+ };
+
+ l();
+}
+#endif
+
+void zeroSizeArrayLambdaCaptureUndefined2() {
+ S arr[0];
+ int n;
+
+ [arr, n]{
+ int x = n; //expected-warning{{Assigned value is garbage or undefined}}
+ (void) x;
+ }();
+}
+
+struct Wrapper{
+ S arr[0];
+};
+
+void zeroSizeArrayMember() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ {
+ Wrapper W;
+ }
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+void zeroSizeArrayMemberCopyMove() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ {
+ Wrapper W;
+ Wrapper W2 = W;
+ Wrapper W3 = (Wrapper&&) W2;
+ }
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
+
+struct MultiWrapper{
+ S arr[2][0];
+};
+
+void zeroSizeMultidimensionalArrayMember() {
+ S::CtorInvocationCount = 0;
+ S::DtorInvocationCount = 0;
+
+ {
+ MultiWrapper MW;
+ }
+
+ clang_analyzer_eval(S::CtorInvocationCount == 0); //expected-warning{{TRUE}}
+ clang_analyzer_eval(S::DtorInvocationCount == 0); //expected-warning{{TRUE}}
+}
More information about the cfe-commits
mailing list