Skip to content

Commit 43eb18e

Browse files
authored
[analyzer] Do list initialization for CXXNewExpr with initializer list arg (#127702)
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 and arrays (like #54910). Lmk if it is preferred to then leave these test cases out for now.
1 parent 3f63e1c commit 43eb18e

File tree

3 files changed

+257
-9
lines changed

3 files changed

+257
-9
lines changed

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2533,6 +2533,15 @@ RegionStoreManager::bind(LimitedRegionBindingsConstRef B, Loc L, SVal V) {
25332533

25342534
const MemRegion *R = MemRegVal->getRegion();
25352535

2536+
// Binding directly to a symbolic region should be treated as binding
2537+
// to element 0.
2538+
if (const auto *SymReg = dyn_cast<SymbolicRegion>(R)) {
2539+
QualType Ty = SymReg->getPointeeStaticType();
2540+
if (Ty->isVoidType())
2541+
Ty = StateMgr.getContext().CharTy;
2542+
R = GetElementZeroRegion(SymReg, Ty);
2543+
}
2544+
25362545
// Check if the region is a struct region.
25372546
if (const TypedValueRegion* TR = dyn_cast<TypedValueRegion>(R)) {
25382547
QualType Ty = TR->getValueType();
@@ -2546,15 +2555,6 @@ RegionStoreManager::bind(LimitedRegionBindingsConstRef B, Loc L, SVal V) {
25462555
return bindAggregate(B, TR, V);
25472556
}
25482557

2549-
// Binding directly to a symbolic region should be treated as binding
2550-
// to element 0.
2551-
if (const auto *SymReg = dyn_cast<SymbolicRegion>(R)) {
2552-
QualType Ty = SymReg->getPointeeStaticType();
2553-
if (Ty->isVoidType())
2554-
Ty = StateMgr.getContext().CharTy;
2555-
R = GetElementZeroRegion(SymReg, Ty);
2556-
}
2557-
25582558
assert((!isa<CXXThisRegion>(R) || !B.lookup(R)) &&
25592559
"'this' pointer is not an l-value and is not assignable");
25602560

clang/test/Analysis/initializer.cpp

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,224 @@ void foo() {
254254
}
255255
} // namespace CXX17_aggregate_construction
256256

257+
namespace newexpr_init_list_initialization {
258+
template <class FirstT, class... Rest>
259+
void escape(FirstT first, Rest... args);
260+
261+
struct S {
262+
int foo;
263+
int bar;
264+
};
265+
void none_designated() {
266+
S *s = new S{13,1};
267+
clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
268+
clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
269+
delete s;
270+
}
271+
void none_designated_swapped() {
272+
S *s = new S{1,13};
273+
clang_analyzer_eval(1 == s->foo); // expected-warning{{TRUE}}
274+
clang_analyzer_eval(13 == s->bar); // expected-warning{{TRUE}}
275+
delete s;
276+
}
277+
void one_designated_one_not() {
278+
S *s = new S{ 1, .bar = 13 };
279+
clang_analyzer_eval(1 == s->foo); // expected-warning{{TRUE}}
280+
clang_analyzer_eval(13 == s->bar); // expected-warning{{TRUE}}
281+
delete s;
282+
}
283+
void all_designated() {
284+
S *s = new S{
285+
.foo = 13,
286+
.bar = 1,
287+
};
288+
clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
289+
clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
290+
delete s;
291+
}
292+
void non_designated_array_of_aggr_struct() {
293+
S *s = new S[2] { {1, 2}, {3, 4} };
294+
clang_analyzer_eval(1 == s[0].foo); // expected-warning{{TRUE}}
295+
clang_analyzer_eval(2 == s[0].bar); // expected-warning{{TRUE}}
296+
clang_analyzer_eval(3 == s[1].foo); // expected-warning{{TRUE}}
297+
clang_analyzer_eval(4 == s[1].bar); // expected-warning{{TRUE}}
298+
delete[] s;
299+
}
300+
301+
struct WithGaps {
302+
int foo;
303+
int bar;
304+
int baz;
305+
};
306+
void out_of_order_designated_initializers_with_gaps() {
307+
WithGaps *s = new WithGaps{
308+
.foo = 13,
309+
.baz = 1,
310+
};
311+
clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
312+
clang_analyzer_eval(0 == s->bar); // expected-warning{{TRUE}}
313+
clang_analyzer_eval(1 == s->baz); // expected-warning{{TRUE}}
314+
delete s;
315+
}
316+
317+
// https://eel.is/c++draft/dcl.init.aggr#note-6:
318+
// Static data members, non-static data members of anonymous
319+
// union members, and unnamed bit-fields are not considered
320+
// elements of the aggregate.
321+
struct NonConsideredFields {
322+
int i;
323+
static int s;
324+
int j;
325+
int :17;
326+
int k;
327+
};
328+
void considered_fields_initd() {
329+
auto S = new NonConsideredFields { 1, 2, 3 };
330+
clang_analyzer_eval(1 == S->i); // expected-warning{{TRUE}}
331+
clang_analyzer_eval(2 == S->j); // expected-warning{{TRUE}}
332+
clang_analyzer_eval(3 == S->k); // expected-warning{{TRUE}}
333+
delete S;
334+
}
335+
336+
#if __cplusplus >= 201703L
337+
enum Enum : int {
338+
};
339+
void list_init_enum() {
340+
Enum *E = new Enum{53};
341+
clang_analyzer_eval(53 == *E); // expected-warning{{TRUE}}
342+
delete E;
343+
}
344+
#endif // __cplusplus >= 201703L
345+
346+
class PubClass {
347+
public:
348+
int foo;
349+
int bar;
350+
};
351+
void public_class_designated_initializers() {
352+
S *s = new S{
353+
.foo = 13,
354+
.bar = 1,
355+
};
356+
clang_analyzer_eval(13 == s->foo); // expected-warning{{TRUE}}
357+
clang_analyzer_eval(1 == s->bar); // expected-warning{{TRUE}}
358+
delete s;
359+
}
360+
361+
union UnionTestTy {
362+
int x;
363+
char y;
364+
};
365+
void new_expr_aggr_init_union_no_designator() {
366+
UnionTestTy *u = new UnionTestTy{};
367+
clang_analyzer_eval(0 == u->x); // expected-warning{{UNKNOWN}} FIXME: should be TRUE
368+
clang_analyzer_eval(u->y); // expected-warning{{UNKNOWN}} FIXME: should be undefined, warning
369+
delete u;
370+
}
371+
void new_expr_aggr_init_union_designated_first_field() {
372+
UnionTestTy *u = new UnionTestTy{ .x = 14 };
373+
clang_analyzer_eval(14 == u->x); // expected-warning{{UNKNOWN}} FIXME: should be TRUE
374+
clang_analyzer_eval(u->y); // expected-warning{{UNKNOWN}} FIXME: should be undefined, warning
375+
delete u;
376+
}
377+
void new_expr_aggr_init_union_designated_non_first_field() {
378+
UnionTestTy *u = new UnionTestTy{ .y = 3 };
379+
clang_analyzer_eval(3 == u->y); // expected-warning{{UNKNOWN}} FIXME: should be TRUE
380+
clang_analyzer_eval(u->x); // expected-warning{{UNKNOWN}} FIXME: should be undefined, warning
381+
delete u;
382+
}
383+
384+
union UnionTestTyWithDefaultMemberInit {
385+
int x;
386+
char y = 14;
387+
};
388+
void union_with_default_member_init_empty_init_list() {
389+
auto U = new UnionTestTyWithDefaultMemberInit{};
390+
// clang_analyzer_eval(14 == U->y); // FIXME: Should be true
391+
clang_analyzer_eval(U->x); // expected-warning{{UNKNOWN}} FIXME: should be undefined, warning
392+
delete U;
393+
}
394+
395+
struct Inner {
396+
int bar;
397+
};
398+
struct Nested {
399+
int foo;
400+
Inner inner;
401+
int baz;
402+
};
403+
void nested_aggregates() {
404+
auto N = new Nested{};
405+
clang_analyzer_eval(0 == N->foo); // expected-warning{{TRUE}}
406+
clang_analyzer_eval(0 == N->inner.bar); // expected-warning{{TRUE}}
407+
clang_analyzer_eval(0 == N->baz); // expected-warning{{TRUE}}
408+
409+
auto N1 = new Nested{1};
410+
clang_analyzer_eval(1 == N1->foo); // expected-warning{{TRUE}}
411+
clang_analyzer_eval(0 == N1->inner.bar); // expected-warning{{TRUE}}
412+
clang_analyzer_eval(0 == N1->baz); // expected-warning{{TRUE}}
413+
414+
auto N2 = new Nested{.baz = 14};
415+
clang_analyzer_eval(0 == N2->foo); // expected-warning{{TRUE}}
416+
clang_analyzer_eval(0 == N2->inner.bar); // expected-warning{{TRUE}}
417+
clang_analyzer_eval(14 == N2->baz); // expected-warning{{TRUE}}
418+
419+
auto N3 = new Nested{1,2,3};
420+
clang_analyzer_eval(1 == N3->foo); // expected-warning{{TRUE}}
421+
clang_analyzer_eval(2 == N3->inner.bar); // expected-warning{{TRUE}}
422+
clang_analyzer_eval(3 == N3->baz); // expected-warning{{TRUE}}
423+
424+
auto N4 = new Nested{1, {}, 3};
425+
clang_analyzer_eval(1 == N4->foo); // expected-warning{{TRUE}}
426+
clang_analyzer_eval(0 == N4->inner.bar); // expected-warning{{TRUE}}
427+
clang_analyzer_eval(3 == N4->baz); // expected-warning{{TRUE}}
428+
429+
auto N5 = new Nested{{},{},{}};
430+
clang_analyzer_eval(0 == N5->foo); // expected-warning{{TRUE}}
431+
clang_analyzer_eval(0 == N5->inner.bar); // expected-warning{{TRUE}}
432+
clang_analyzer_eval(0 == N5->baz); // expected-warning{{TRUE}}
433+
434+
auto N6 = new Nested{1, {.bar = 2}, 3};
435+
clang_analyzer_eval(1 == N6->foo); // expected-warning{{TRUE}}
436+
clang_analyzer_eval(2 == N6->inner.bar); // expected-warning{{TRUE}}
437+
clang_analyzer_eval(3 == N6->baz); // expected-warning{{TRUE}}
438+
439+
auto N7 = new Nested{1, {2}, 3};
440+
clang_analyzer_eval(1 == N7->foo); // expected-warning{{TRUE}}
441+
clang_analyzer_eval(2 == N7->inner.bar); // expected-warning{{TRUE}}
442+
clang_analyzer_eval(3 == N7->baz); // expected-warning{{TRUE}}
443+
444+
escape(N,N1,N2,N3,N4,N5,N6,N7);
445+
}
446+
} // namespace newexpr_init_list_initialization
447+
448+
namespace placement_new_initializer_list_arg {
449+
struct S {
450+
int x;
451+
};
452+
void aggregate_struct() {
453+
S s;
454+
S *s_ptr = new (&s) S{1};
455+
clang_analyzer_eval(1 == s_ptr->x); // expected-warning{{TRUE}}
456+
457+
S vi;
458+
S *vi_ptr = new (&vi) S{};
459+
clang_analyzer_eval(0 == vi_ptr->x); // expected-warning{{TRUE}}
460+
461+
S di;
462+
S *di_ptr = new (&di) S;
463+
int z = di_ptr->x + 1; // expected-warning{{The left operand of '+' is a garbage value}}
464+
}
465+
void initialize_non_zeroth_element(S arr[2]) {
466+
S *s = new (&arr[1]) S{1};
467+
clang_analyzer_eval(1 == s->x); // expected-warning{{TRUE}}
468+
}
469+
void initialize_non_zeroth_argument_pointers(S *arr[2]) {
470+
arr[1] = new (arr[1]) S{1};
471+
clang_analyzer_eval(1 == arr[1]->x); // expected-warning{{TRUE}}
472+
}
473+
} // namespace placement_new_initializer_list_arg
474+
257475
namespace CXX17_transparent_init_list_exprs {
258476
class A {};
259477

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// RUN: %clang_analyze_cc1 -verify %s\
2+
// RUN: -analyzer-checker=core,debug.ExprInspection
3+
4+
void clang_analyzer_eval(bool);
5+
6+
using size_t = decltype(sizeof(int));
7+
8+
template <class FirstT, class... Rest>
9+
void escape(FirstT first, Rest... args);
10+
11+
namespace CustomClassType {
12+
struct S {
13+
int x;
14+
static void* operator new(size_t size) {
15+
return ::operator new(size);
16+
}
17+
};
18+
void F() {
19+
S *s = new S;
20+
clang_analyzer_eval(s->x); // expected-warning{{UNKNOWN}} FIXME: should be an undefined warning
21+
22+
S *s2 = new S{};
23+
clang_analyzer_eval(0 == s2->x); // expected-warning{{TRUE}}
24+
25+
S *s3 = new S{1};
26+
clang_analyzer_eval(1 == s3->x); // expected-warning{{TRUE}}
27+
28+
escape(s, s2, s3);
29+
}
30+
} // namespace CustomClassType

0 commit comments

Comments
 (0)