Skip to content

Commit 4610e5c

Browse files
authored
[analyzer] Don't copy field-by-field conjured LazyCompoundVals (2/4) (#115917)
Split from #114835
1 parent 4cdfa2a commit 4610e5c

File tree

3 files changed

+43
-7
lines changed

3 files changed

+43
-7
lines changed

clang/lib/StaticAnalyzer/Core/RegionStore.cpp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,9 @@ class RegionStoreManager : public StoreManager {
608608
return getBinding(getRegionBindings(S), L, T);
609609
}
610610

611+
std::optional<SVal>
612+
getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const;
613+
611614
std::optional<SVal> getDefaultBinding(Store S, const MemRegion *R) override {
612615
RegionBindingsRef B = getRegionBindings(S);
613616
// Default bindings are always applied over a base region so look up the
@@ -2605,9 +2608,43 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
26052608
return NewB;
26062609
}
26072610

2611+
std::optional<SVal>
2612+
RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
2613+
const MemRegion *BaseR = LCV.getRegion();
2614+
2615+
// We only handle base regions.
2616+
if (BaseR != BaseR->getBaseRegion())
2617+
return std::nullopt;
2618+
2619+
const auto *Cluster = getRegionBindings(LCV.getStore()).lookup(BaseR);
2620+
if (!Cluster || !llvm::hasSingleElement(*Cluster))
2621+
return std::nullopt;
2622+
2623+
const auto [Key, Value] = *Cluster->begin();
2624+
return Key.isDirect() ? std::optional<SVal>{} : Value;
2625+
}
2626+
26082627
std::optional<RegionBindingsRef> RegionStoreManager::tryBindSmallStruct(
26092628
RegionBindingsConstRef B, const TypedValueRegion *R, const RecordDecl *RD,
26102629
nonloc::LazyCompoundVal LCV) {
2630+
// If we try to copy a Conjured value representing the value of the whole
2631+
// struct, don't try to element-wise copy each field.
2632+
// That would unnecessarily bind Derived symbols slicing off the subregion for
2633+
// the field from the whole Conjured symbol.
2634+
//
2635+
// struct Window { int width; int height; };
2636+
// Window getWindow(); <-- opaque fn.
2637+
// Window w = getWindow(); <-- conjures a new Window.
2638+
// Window w2 = w; <-- trivial copy "w", calling "tryBindSmallStruct"
2639+
//
2640+
// We should not end up with a new Store for "w2" like this:
2641+
// Direct [ 0..31]: Derived{Conj{}, w.width}
2642+
// Direct [32..63]: Derived{Conj{}, w.height}
2643+
// Instead, we should just bind that Conjured value instead.
2644+
if (std::optional<SVal> Val = getUniqueDefaultBinding(LCV)) {
2645+
return B.addBinding(BindingKey::Make(R, BindingKey::Default), Val.value());
2646+
}
2647+
26112648
FieldVector Fields;
26122649

26132650
if (const CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(RD))

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,9 @@ void _02_structs_with_members() {
8383
clang_analyzer_dump_val(Aggr2); // expected-warning {{lazyCompoundVal}}
8484
clang_analyzer_dump_val(Aggr3); // expected-warning {{lazyCompoundVal}}
8585

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).
86+
// We should have the same Conjured symbol for "Aggr", "Aggr2" and "Aggr3".
87+
// We used to have Derived symbols for the individual fields that were
88+
// copied as part of copying the whole struct.
8889
clang_analyzer_printState();
8990
// CHECK: "store": { "pointer": "0x{{[0-9a-f]+}}", "items": [
9091
// CHECK-NEXT: { "cluster": "GlobalInternalSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
@@ -97,12 +98,10 @@ void _02_structs_with_members() {
9798
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ:conj_\$[0-9]+{int, LC[0-9]+, S[0-9]+, #[0-9]+}]]" }
9899
// CHECK-NEXT: ]},
99100
// 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}" }
101+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ]]" }
102102
// CHECK-NEXT: ]},
103103
// 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}" }
104+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ]]" }
106105
// CHECK-NEXT: ]}
107106
// CHECK-NEXT: ]},
108107

clang/test/Analysis/store-dump-orders.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ void test_output(int n) {
4141
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
4242
// CHECK-NEXT: ]},
4343
// CHECK-NEXT: { "cluster": "objfirst", "pointer": "0x{{[0-9a-f]+}}", "items": [
44-
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "lazyCompoundVal
44+
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
4545
// CHECK-NEXT: { "kind": "Direct", "offset": 320, "value": "1 S32b" },
4646
// CHECK-NEXT: { "kind": "Direct", "offset": 352, "value": "2 S32b" },
4747
// CHECK-NEXT: { "kind": "Direct", "offset": 384, "value": "3 S32b" }

0 commit comments

Comments
 (0)