Skip to content

Commit b96c24b

Browse files
steakhalsteakhal
authored
[analyzer] Allow copying empty structs (1/4) (#115916)
We represent copies of structs by LazyCompoundVals, that is basically a snapshot of the Store and Region that your copy would refer to. This snapshot is actually not taken for empty structs (structs that have no non-static data members), because the users won't be able to access any fields anyways, so why bother. However, when it comes to taint propagation, it would be nice if instances of empty structs would behave similar to non-empty structs. For this, we need an identity for which taint can bind, so Unknown - that was used in the past wouldn't work. Consequently, copying the value of an empty struct should behave the same way as a non-empty struct, thus be represented by a LazyCompoundVal. Split from #114835
1 parent 43bef75 commit b96c24b

File tree

2 files changed

+99
-16
lines changed

2 files changed

+99
-16
lines changed

‎clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2336,20 +2336,16 @@ NonLoc RegionStoreManager::createLazyBinding(RegionBindingsConstRef B,
23362336
return svalBuilder.makeLazyCompoundVal(StoreRef(B.asStore(), *this), R);
23372337
}
23382338

2339-
static bool isRecordEmpty(const RecordDecl *RD) {
2340-
if (!RD->field_empty())
2341-
return false;
2342-
if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(RD))
2343-
return CRD->getNumBases() == 0;
2344-
return true;
2345-
}
2346-
23472339
SVal RegionStoreManager::getBindingForStruct(RegionBindingsConstRef B,
23482340
const TypedValueRegion *R) {
23492341
const RecordDecl *RD = R->getValueType()->castAs<RecordType>()->getDecl();
2350-
if (!RD->getDefinition() || isRecordEmpty(RD))
2342+
if (!RD->getDefinition())
23512343
return UnknownVal();
23522344

2345+
// We also create a LCV for copying empty structs because then the store
2346+
// behavior doesn't depend on the struct layout.
2347+
// This way even an empty struct can carry taint, no matter if creduce drops
2348+
// the last field member or not.
23532349
return createLazyBinding(B, R);
23542350
}
23552351

‎clang/test/Analysis/ctor-trivial-copy.cpp

Lines changed: 94 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=constructors -verify %s
1+
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-inlining=constructors -verify %s \
2+
// RUN: 2>&1 | FileCheck %s
23

34

4-
template<typename T>
5-
void clang_analyzer_dump(T&);
5+
void clang_analyzer_printState();
6+
template<typename T> void clang_analyzer_dump_lref(T&);
7+
template<typename T> void clang_analyzer_dump_val(T);
8+
template <typename T> T conjure();
9+
template <typename... Ts> void nop(const Ts &... args) {}
610

711
struct aggr {
812
int x;
@@ -15,20 +19,103 @@ struct empty {
1519
void test_copy_return() {
1620
aggr s1 = {1, 2};
1721
aggr const& cr1 = aggr(s1);
18-
clang_analyzer_dump(cr1); // expected-warning-re {{&lifetime_extended_object{aggr, cr1, S{{[0-9]+}}} }}
22+
clang_analyzer_dump_lref(cr1); // expected-warning-re {{&lifetime_extended_object{aggr, cr1, S{{[0-9]+}}} }}
1923

2024
empty s2;
2125
empty const& cr2 = empty{s2};
22-
clang_analyzer_dump(cr2); // expected-warning-re {{&lifetime_extended_object{empty, cr2, S{{[0-9]+}}} }}
26+
clang_analyzer_dump_lref(cr2); // expected-warning-re {{&lifetime_extended_object{empty, cr2, S{{[0-9]+}}} }}
2327
}
2428

2529
void test_assign_return() {
2630
aggr s1 = {1, 2};
2731
aggr d1;
28-
clang_analyzer_dump(d1 = s1); // expected-warning {{&d1 }}
32+
clang_analyzer_dump_lref(d1 = s1); // expected-warning {{&d1 }}
2933

3034
empty s2;
3135
empty d2;
32-
clang_analyzer_dump(d2 = s2); // expected-warning {{&d2 }} was Unknown
36+
clang_analyzer_dump_lref(d2 = s2); // expected-warning {{&d2 }} was Unknown
3337
}
3438

39+
40+
namespace trivial_struct_copy {
41+
42+
void _01_empty_structs() {
43+
clang_analyzer_dump_val(conjure<empty>()); // expected-warning {{lazyCompoundVal}}
44+
empty Empty = conjure<empty>();
45+
empty Empty2 = Empty;
46+
empty Empty3 = Empty2;
47+
// All of these should refer to the exact same LCV, because all of
48+
// these trivial copies refer to the original conjured value.
49+
// There were Unknown before:
50+
clang_analyzer_dump_val(Empty); // expected-warning {{lazyCompoundVal}}
51+
clang_analyzer_dump_val(Empty2); // expected-warning {{lazyCompoundVal}}
52+
clang_analyzer_dump_val(Empty3); // expected-warning {{lazyCompoundVal}}
53+
54+
// Notice that we don't have entries for the copies, only for the original "Empty" object.
55+
// That binding was added by the opaque "conjure" call, directly constructing to the variable "Empty" by copy-elision.
56+
// The copies are not present because the ExprEngine skips the evalBind of empty structs.
57+
// And even if such binds would reach the Store, the Store copies small structs by copying the individual fields,
58+
// and there are no fields within "Empty", thus we wouldn't have any entries anyways.
59+
clang_analyzer_printState();
60+
// CHECK: "store": { "pointer": "0x{{[0-9a-f]+}}", "items": [
61+
// CHECK-NEXT: { "cluster": "GlobalInternalSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
62+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
63+
// CHECK-NEXT: ]},
64+
// CHECK-NEXT: { "cluster": "GlobalSystemSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
65+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
66+
// CHECK-NEXT: ]},
67+
// CHECK-NEXT: { "cluster": "Empty", "pointer": "0x{{[0-9a-f]+}}", "items": [
68+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[EMPTY_CONJ:conj_\$[0-9]+{int, LC[0-9]+, S[0-9]+, #[0-9]+}]]" }
69+
// CHECK-NEXT: ]}
70+
// CHECK-NEXT: ]},
71+
72+
nop(Empty, Empty2, Empty3);
73+
}
74+
75+
void _02_structs_with_members() {
76+
clang_analyzer_dump_val(conjure<aggr>()); // expected-warning {{lazyCompoundVal}}
77+
aggr Aggr = conjure<aggr>();
78+
aggr Aggr2 = Aggr;
79+
aggr Aggr3 = Aggr2;
80+
// All of these should refer to the exact same LCV, because all of
81+
// these trivial copies refer to the original conjured value.
82+
clang_analyzer_dump_val(Aggr); // expected-warning {{lazyCompoundVal}}
83+
clang_analyzer_dump_val(Aggr2); // expected-warning {{lazyCompoundVal}}
84+
clang_analyzer_dump_val(Aggr3); // expected-warning {{lazyCompoundVal}}
85+
86+
// We have fields in the struct we copy, thus we also have the entries for the copies
87+
// (and for all of their fields).
88+
clang_analyzer_printState();
89+
// CHECK: "store": { "pointer": "0x{{[0-9a-f]+}}", "items": [
90+
// CHECK-NEXT: { "cluster": "GlobalInternalSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
91+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
92+
// CHECK-NEXT: ]},
93+
// CHECK-NEXT: { "cluster": "GlobalSystemSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
94+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
95+
// CHECK-NEXT: ]},
96+
// CHECK-NEXT: { "cluster": "Aggr", "pointer": "0x{{[0-9a-f]+}}", "items": [
97+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ:conj_\$[0-9]+{int, LC[0-9]+, S[0-9]+, #[0-9]+}]]" }
98+
// CHECK-NEXT: ]},
99+
// CHECK-NEXT: { "cluster": "Aggr2", "pointer": "0x{{[0-9a-f]+}}", "items": [
100+
// CHECK-NEXT: { "kind": "Direct", "offset": 0, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.x}" },
101+
// CHECK-NEXT: { "kind": "Direct", "offset": 32, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.y}" }
102+
// CHECK-NEXT: ]},
103+
// CHECK-NEXT: { "cluster": "Aggr3", "pointer": "0x{{[0-9a-f]+}}", "items": [
104+
// CHECK-NEXT: { "kind": "Direct", "offset": 0, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.x}" },
105+
// CHECK-NEXT: { "kind": "Direct", "offset": 32, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.y}" }
106+
// CHECK-NEXT: ]}
107+
// CHECK-NEXT: ]},
108+
109+
nop(Aggr, Aggr2, Aggr3);
110+
}
111+
112+
// Tests that use `clang_analyzer_printState()` must share the analysis entry
113+
// point, and have a strict ordering between. This is to meet the different
114+
// `clang_analyzer_printState()` calls in a fixed relative ordering, thus
115+
// FileCheck could check the stdouts.
116+
void entrypoint() {
117+
_01_empty_structs();
118+
_02_structs_with_members();
119+
}
120+
121+
} // namespace trivial_struct_copy

0 commit comments

Comments
 (0)