[analyzer] Do list initialization for CXXNewExpr with initializer list arg #127702
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Michael Flanders (Flandini) ChangesFixes #116444. Closed #127700 because I accidentally updated it in github UI. Current vs expected behaviorPreviously, the result of a In this example: there would be a binding of ChangesThis 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. llvm-project/clang/lib/StaticAnalyzer/Core/RegionStore.cpp Lines 2426 to 2448 in 8529bd7 Followup workThis PR only makes CSA do list init for I've added some straightforward test cases here for the Full diff: https://github.com/llvm/llvm-project/pull/127702.diff 3 Files Affected:
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 {};
|
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Michael Flanders (Flandini) ChangesFixes #116444. Closed #127700 because I accidentally updated it in github UI. Current vs expected behaviorPreviously, the result of a In this example: there would be a binding of ChangesThis 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. llvm-project/clang/lib/StaticAnalyzer/Core/RegionStore.cpp Lines 2426 to 2448 in 8529bd7 Followup workThis PR only makes CSA do list init for I've added some straightforward test cases here for the Full diff: https://github.com/llvm/llvm-project/pull/127702.diff 3 Files Affected:
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 {};
|
Flandini
force-pushed
the
flandini/init-list-new-expr-3
branch
from
e638667 to
12791f2
Compare
|
Going to add tests for placement new and user-defined placement new operator with list initializers also |
Xazax-hun
approved these changes
Feb 19, 2025
There was a problem hiding this comment.
This change looks good to me but the code makes me wonder if we correctly handle placement new.
What about cases like doing placement new into a non-zero'th element of a region like:
void f(S *a[]) {
s[1] = new (S[1]) S{1, 2};
}
If this works as expected, that is great! If it does not, I am OK doing something about it in a follow-up PR.
steakhal
approved these changes
Feb 19, 2025
steakhal
requested changes
Feb 20, 2025
|
Haven't had enough free time to add tests for placement new yet. Like Xazax-hun said above, I want to at least try to add some tests to know the status of it with this PR before merging. |
steakhal
reviewed
Feb 21, 2025
|
Yes, ready to merge, sorry for the wait. Added a few placement new list initializer tests just now. Seems to work as well for placement new. |
steakhal
reviewed
Feb 21, 2025
There was a problem hiding this comment.
Technically, shouldn't one only use the return value of the placement new? Using the original object is technically UB (I know we accept it, and do the right thing, but anyways) maybe we should have a test demonstrating theat it would work with the ret val of placement new too.
|
Didn't know it was UB, fixed. |
steakhal
approved these changes
Feb 28, 2025
|
Can I merge this @Flandini now? |
|
Yes, fine to merge |
cheezeburglar
pushed a commit
to cheezeburglar/llvm-project
that referenced
this pull request
Feb 28, 2025
…t arg (llvm#127702) Fixes llvm#116444. Closed llvm#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 llvm#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 and arrays (like llvm#54910). Lmk if it is preferred to then leave these test cases out for now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #116444.
Closed #127700 because I accidentally updated it in github UI.
Current vs expected behavior
Previously, the result of a
CXXNewExprwas not always list initialized when using an initializer list.In this example:
there would be a binding of
stocompoundVal{1}, but this isn't used during later field binding lookup. After this PR, there is instead a binding ofs->xto1. 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.
llvm-project/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
Lines 2426 to 2448 in 8529bd7
Followup work
This PR only makes CSA do list init for
CXXNewExprs. After this, I would like to make some changes toRegionStoreMananger::bindin how it handles list initialization generally.I've added some straightforward test cases here for the
newexpr with a list initializer. I started adding some more before realizing that the current general (not justnewexpr) list initialization could be changed to handle more cases like list initialization of unions and arrays (like #54910). Lmk if it is preferred to then leave these test cases out for now.