[clang] [analyzer] Do list initialization for CXXNewExpr with initializer list arg (PR #127702)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 18 13:55:57 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Michael Flanders (Flandini)
<details>
<summary>Changes</summary>
Fixes #<!-- -->116444.
Closed #<!-- -->127700 because I accidentally updated it in github UI.
### Current vs expected behavior
Previously, the result of a `CXXNewExpr` was not always list initialized when using an initializer list.
In this example:
```
struct S { int x; };
void F() {
S *s = new S{1};
delete s;
}
```
there would be a binding of `s` to `compoundVal{1}`, but this isn't used during later field binding lookup. After this PR, there is instead a binding of `s->x` to `1`. This is the cause of #<!-- -->116444 since the field binding lookup returns undefined in some cases currently.
### Changes
This PR swaps around the handling of typed value regions (seems to be the usual region type when doing non-CXX-new-expr list initialization) and symbolic regions (the result of the CXX new expr), so that symbolic regions also get list initialized. In the below snippet, it swaps the order of the two conditionals.
https://github.com/llvm/llvm-project/blob/8529bd7b964cc9fafe8fece84f7bd12dacb09560/clang/lib/StaticAnalyzer/Core/RegionStore.cpp#L2426-L2448
### Followup work
This PR only makes CSA do list init for `CXXNewExpr`s. After this, I would like to make some changes to `RegionStoreMananger::bind` in how it handles list initialization generally.
I've added some straightforward test cases here for the `new` expr with a list initializer. I started adding some more before realizing that the current general (not just `new` expr) list initialization could be changed to handle more cases like list initialization of unions, nested aggregates, and arrays (like https://github.com/llvm/llvm-project/issues/54910). There also seems to be an issue with nested aggregates. Lmk if it is preferred to then leave these test cases out for now.
---
Full diff: https://github.com/llvm/llvm-project/pull/127702.diff
3 Files Affected:
- (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp (+27-7)
- (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+9-9)
- (modified) clang/test/Analysis/initializer.cpp (+185)
``````````diff
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
index f7020da2e6da2..c5c0f34d0383d 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -1059,16 +1059,36 @@ void ExprEngine::VisitCXXNewExpr(const CXXNewExpr *CNE, ExplodedNode *Pred,
if (!NewN)
return;
- // If the type is not a record, we won't have a CXXConstructExpr as an
- // initializer. Copy the value over.
- if (const Expr *Init = CNE->getInitializer()) {
- if (!isa<CXXConstructExpr>(Init)) {
- assert(Bldr.getResults().size() == 1);
+ const Expr *Init = CNE->getInitializer();
+ if (!Init)
+ return;
+
+ if (const InitListExpr *ILE = dyn_cast<InitListExpr>(Init)) {
+ QualType AllocType = CNE->getAllocatedType();
+ // Semantic form of ILE means that all members have an initializer present
+ // in the ILE
+ if (AllocType->isAggregateType() && ILE->isSemanticForm()) {
+ const CXXRecordDecl *Record = AllocType->getAsCXXRecordDecl();
+ auto ZipIter = llvm::zip_equal(Record->fields(), ILE->children());
+ for (auto [FD, InitExpr] : ZipIter) {
+ SVal FieldLVal = State->getLValue(FD, Result);
+ SVal InitSVal = State->getSVal(InitExpr, LCtx);
+ State = State->bindLoc(FieldLVal, InitSVal, LCtx);
+ }
Bldr.takeNodes(NewN);
- evalBind(Dst, CNE, NewN, Result, State->getSVal(Init, LCtx),
- /*FirstInit=*/IsStandardGlobalOpNewFunction);
+ Bldr.generateNode(CNE, NewN, State);
+ return;
}
}
+
+ // If the type is not a record, we won't have a CXXConstructExpr as an
+ // initializer. Copy the value over.
+ if (!isa<CXXConstructExpr>(Init)) {
+ assert(Bldr.getResults().size() == 1);
+ Bldr.takeNodes(NewN);
+ evalBind(Dst, CNE, NewN, Result, State->getSVal(Init, LCtx),
+ /*FirstInit=*/IsStandardGlobalOpNewFunction);
+ }
}
void ExprEngine::VisitCXXDeleteExpr(const CXXDeleteExpr *CDE,
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index d01b6ae55f611..e376b84f8219f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2425,6 +2425,15 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
const MemRegion *R = MemRegVal->getRegion();
+ // Binding directly to a symbolic region should be treated as binding
+ // to element 0.
+ if (const auto *SymReg = dyn_cast<SymbolicRegion>(R)) {
+ QualType Ty = SymReg->getPointeeStaticType();
+ if (Ty->isVoidType())
+ Ty = StateMgr.getContext().CharTy;
+ R = GetElementZeroRegion(SymReg, Ty);
+ }
+
// Check if the region is a struct region.
if (const TypedValueRegion* TR = dyn_cast<TypedValueRegion>(R)) {
QualType Ty = TR->getValueType();
@@ -2438,15 +2447,6 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc L, SVal V) {
return bindAggregate(B, TR, V);
}
- // Binding directly to a symbolic region should be treated as binding
- // to element 0.
- if (const auto *SymReg = dyn_cast<SymbolicRegion>(R)) {
- QualType Ty = SymReg->getPointeeStaticType();
- if (Ty->isVoidType())
- Ty = StateMgr.getContext().CharTy;
- R = GetElementZeroRegion(SymReg, Ty);
- }
-
assert((!isa<CXXThisRegion>(R) || !B.lookup(R)) &&
"'this' pointer is not an l-value and is not assignable");
diff --git a/clang/test/Analysis/initializer.cpp b/clang/test/Analysis/initializer.cpp
index f50afff25d245..edc41d29e1df1 100644
--- a/clang/test/Analysis/initializer.cpp
+++ b/clang/test/Analysis/initializer.cpp
@@ -254,6 +254,191 @@ void foo() {
}
} // namespace CXX17_aggregate_construction
+namespace newexpr_init_list_initialization {
+struct S {
+ int foo;
+ int bar;
+};
+void none_designated() {
+ S *s = new S{13,1};
+ clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
+ delete s;
+}
+void none_designated_swapped() {
+ S *s = new S{1,13};
+ clang_analyzer_eval(1 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(13 == s->bar); // expected-warning{{TRUE}}
+ delete s;
+}
+void one_designated_one_not() {
+ S *s = new S{ 1, .bar = 13 };
+ clang_analyzer_eval(1 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(13 == s->bar); // expected-warning{{TRUE}}
+ delete s;
+}
+void all_designated() {
+ S *s = new S{
+ .foo = 13,
+ .bar = 1,
+ };
+ clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
+ delete s;
+}
+void non_designated_array_of_aggr_struct() {
+ S *s = new S[2] { {1, 2}, {3, 4} };
+ clang_analyzer_eval(1 == s[0].foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(2 == s[0].bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(3 == s[1].foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(4 == s[1].bar); // expected-warning{{TRUE}}
+ delete[] s;
+}
+
+struct WithGaps {
+ int foo;
+ int bar;
+ int baz;
+};
+void out_of_order_designated_initializers_with_gaps() {
+ WithGaps *s = new WithGaps{
+ .foo = 13,
+ .baz = 1,
+ };
+ clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == s->bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(1 == s->baz); // expected-warning{{TRUE}}
+ delete s;
+}
+
+// https://eel.is/c++draft/dcl.init.aggr#note-6:
+// Static data members, non-static data members of anonymous
+// union members, and unnamed bit-fields are not considered
+// elements of the aggregate.
+struct NonConsideredFields {
+ int i;
+ static int s;
+ int j;
+ int :17;
+ int k;
+};
+void considered_fields_initd() {
+ auto S = new NonConsideredFields { 1, 2, 3 };
+ clang_analyzer_eval(1 == S->i); // expected-warning{{TRUE}}
+ clang_analyzer_eval(2 == S->j); // expected-warning{{TRUE}}
+ clang_analyzer_eval(3 == S->k); // expected-warning{{TRUE}}
+ delete S;
+}
+
+class PubClass {
+public:
+ int foo;
+ int bar;
+};
+void public_class_designated_initializers() {
+ S *s = new S{
+ .foo = 13,
+ .bar = 1,
+ };
+ clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
+ delete s;
+}
+
+union UnionTestTy {
+ int x;
+ char y;
+};
+void new_expr_aggr_init_union_no_designator() {
+ UnionTestTy *u = new UnionTestTy{};
+ clang_analyzer_eval(0 == u->x); // expected-warning{{UNKNOWN}} TODO: should be TRUE
+ clang_analyzer_eval(u->y); // expected-warning{{UNKNOWN}} TODO: should be undefined, warning
+ delete u;
+}
+void new_expr_aggr_init_union_designated_first_field() {
+ UnionTestTy *u = new UnionTestTy{ .x = 14 };
+ clang_analyzer_eval(14 == u->x); // expected-warning{{UNKNOWN}} TODO: should be TRUE
+ clang_analyzer_eval(u->y); // expected-warning{{UNKNOWN}} TODO: should be undefined, warning
+ delete u;
+}
+void new_expr_aggr_init_union_designated_non_first_field() {
+ UnionTestTy *u = new UnionTestTy{ .y = 3 };
+ clang_analyzer_eval(3 == u->y); // expected-warning{{UNKNOWN}} TODO: should be TRUE
+ clang_analyzer_eval(u->x); // expected-warning{{UNKNOWN}} TODO: should be undefined, warning
+ delete u;
+}
+
+union UnionTestTyWithDefaultMemberInit {
+ int x;
+ char y = 14;
+};
+void union_with_default_member_init_empty_init_list() {
+ auto U = new UnionTestTyWithDefaultMemberInit{};
+ // clang_analyzer_eval(14 == U->y); // TODO: Should be true
+ clang_analyzer_eval(U->x); // expected-warning{{UNKNOWN}} TODO: should be undefined, warning
+ delete U;
+}
+
+struct Inner {
+ int bar;
+};
+struct Nested {
+ int foo;
+ Inner inner;
+ int baz;
+};
+void nested_aggregates() {
+ auto N = new Nested{};
+ clang_analyzer_eval(0 == N->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N->inner.bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N->baz); // expected-warning{{TRUE}}
+
+ auto N1 = new Nested{1};
+ clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N1->inner.bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N1->baz); // expected-warning{{TRUE}}
+
+ auto N2 = new Nested{.baz = 14};
+ clang_analyzer_eval(0 == N->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N->inner.bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(14 == N->baz); // expected-warning{{FALSE}} TODO: Should be TRUE
+
+ auto N3 = new Nested{1,2,3};
+ clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(2 == N1->inner.bar); // expected-warning{{FALSE}} TODO: Should be TRUE
+ clang_analyzer_eval(3 == N1->baz); // expected-warning{{FALSE}} TODO: Should be TRUE
+
+ auto N4 = new Nested{1, {}, 3};
+ clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N1->inner.bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(3 == N1->baz); // expected-warning{{FALSE}} TODO: Should be TRUE
+
+ auto N5 = new Nested{{},{},{}};
+ clang_analyzer_eval(0 == N1->foo); // expected-warning{{FALSE}} TODO: Should be TRUE
+ clang_analyzer_eval(0 == N1->inner.bar); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 == N1->baz); // expected-warning{{TRUE}}
+
+ auto N6 = new Nested{1, {.bar = 2}, 3};
+ clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(2 == N1->inner.bar); // expected-warning{{FALSE}} TODO: Should be TRUE
+ clang_analyzer_eval(3 == N1->baz); // expected-warning{{FALSE}} TODO: Should be TRUE
+
+ auto N7 = new Nested{1, {2}, 3};
+ clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
+ clang_analyzer_eval(2 == N1->inner.bar); // expected-warning{{FALSE}} TODO: Should be TRUE
+ clang_analyzer_eval(3 == N1->baz); // expected-warning{{FALSE}} TODO: Should be TRUE
+
+ delete N;
+ delete N1;
+ delete N2;
+ delete N3;
+ delete N4;
+ delete N5;
+ delete N6;
+ delete N7;
+}
+} // namespace newexpr_init_list_initialization
+
namespace CXX17_transparent_init_list_exprs {
class A {};
``````````
</details>
https://github.com/llvm/llvm-project/pull/127702
More information about the cfe-commits
mailing list