Skip to content

[analyzer] Don't copy field-by-field conjured LazyCompoundVals (2/4) #115917

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions clang/lib/StaticAnalyzer/Core/RegionStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,9 @@ class RegionStoreManager : public StoreManager {
return getBinding(getRegionBindings(S), L, T);
}

std::optional<SVal>
getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const;

std::optional<SVal> getDefaultBinding(Store S, const MemRegion *R) override {
RegionBindingsRef B = getRegionBindings(S);
// Default bindings are always applied over a base region so look up the
Expand Down Expand Up @@ -2605,9 +2608,43 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
return NewB;
}

std::optional<SVal>
RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
const MemRegion *BaseR = LCV.getRegion();

// We only handle base regions.
if (BaseR != BaseR->getBaseRegion())
return std::nullopt;

const auto *Cluster = getRegionBindings(LCV.getStore()).lookup(BaseR);
if (!Cluster || !llvm::hasSingleElement(*Cluster))
return std::nullopt;

const auto [Key, Value] = *Cluster->begin();
return Key.isDirect() ? std::optional<SVal>{} : Value;
}

std::optional<RegionBindingsRef> RegionStoreManager::tryBindSmallStruct(
RegionBindingsConstRef B, const TypedValueRegion *R, const RecordDecl *RD,
nonloc::LazyCompoundVal LCV) {
// If we try to copy a Conjured value representing the value of the whole
// struct, don't try to element-wise copy each field.
// That would unnecessarily bind Derived symbols slicing off the subregion for
// the field from the whole Conjured symbol.
//
// struct Window { int width; int height; };
// Window getWindow(); <-- opaque fn.
// Window w = getWindow(); <-- conjures a new Window.
// Window w2 = w; <-- trivial copy "w", calling "tryBindSmallStruct"
//
// We should not end up with a new Store for "w2" like this:
// Direct [ 0..31]: Derived{Conj{}, w.width}
// Direct [32..63]: Derived{Conj{}, w.height}
// Instead, we should just bind that Conjured value instead.
if (std::optional<SVal> Val = getUniqueDefaultBinding(LCV)) {
return B.addBinding(BindingKey::Make(R, BindingKey::Default), Val.value());
}

FieldVector Fields;

if (const CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(RD))
Expand Down
11 changes: 5 additions & 6 deletions clang/test/Analysis/ctor-trivial-copy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,9 @@ void _02_structs_with_members() {
clang_analyzer_dump_val(Aggr2); // expected-warning {{lazyCompoundVal}}
clang_analyzer_dump_val(Aggr3); // expected-warning {{lazyCompoundVal}}

// We have fields in the struct we copy, thus we also have the entries for the copies
// (and for all of their fields).
// We should have the same Conjured symbol for "Aggr", "Aggr2" and "Aggr3".
// We used to have Derived symbols for the individual fields that were
// copied as part of copying the whole struct.
clang_analyzer_printState();
// CHECK: "store": { "pointer": "0x{{[0-9a-f]+}}", "items": [
// CHECK-NEXT: { "cluster": "GlobalInternalSpaceRegion", "pointer": "0x{{[0-9a-f]+}}", "items": [
Expand All @@ -97,12 +98,10 @@ void _02_structs_with_members() {
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ:conj_\$[0-9]+{int, LC[0-9]+, S[0-9]+, #[0-9]+}]]" }
// CHECK-NEXT: ]},
// CHECK-NEXT: { "cluster": "Aggr2", "pointer": "0x{{[0-9a-f]+}}", "items": [
// CHECK-NEXT: { "kind": "Direct", "offset": 0, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.x}" },
// CHECK-NEXT: { "kind": "Direct", "offset": 32, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.y}" }
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ]]" }
// CHECK-NEXT: ]},
// CHECK-NEXT: { "cluster": "Aggr3", "pointer": "0x{{[0-9a-f]+}}", "items": [
// CHECK-NEXT: { "kind": "Direct", "offset": 0, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.x}" },
// CHECK-NEXT: { "kind": "Direct", "offset": 32, "value": "derived_${{[0-9]+}}{[[AGGR_CONJ]],Aggr.y}" }
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "[[AGGR_CONJ]]" }
// CHECK-NEXT: ]}
// CHECK-NEXT: ]},

Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/store-dump-orders.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ void test_output(int n) {
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
// CHECK-NEXT: ]},
// CHECK-NEXT: { "cluster": "objfirst", "pointer": "0x{{[0-9a-f]+}}", "items": [
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "lazyCompoundVal
// CHECK-NEXT: { "kind": "Default", "offset": 0, "value": "conj_$
// CHECK-NEXT: { "kind": "Direct", "offset": 320, "value": "1 S32b" },
// CHECK-NEXT: { "kind": "Direct", "offset": 352, "value": "2 S32b" },
// CHECK-NEXT: { "kind": "Direct", "offset": 384, "value": "3 S32b" }
Expand Down
Loading