[clang] [analyzer] Don't copy field-by-field conjured LazyCompoundVals (2/4) (PR #115917)
Balazs Benics via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 14 06:46:53 PST 2024
https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/115917
>From 548ad576b27e7ecfa29a78c13db5ef04c1ea8e7c Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Sun, 3 Nov 2024 09:41:33 +0100
Subject: [PATCH 1/2] [analyzer] Don't copy field-by-field conjured
LazyCompoundVals (2/4)
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 39 +++++++++++++++++++
clang/test/Analysis/ctor-trivial-copy.cpp | 11 +++---
clang/test/Analysis/store-dump-orders.cpp | 2 +-
3 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index f46282a73fbe40..b54ba43ff42414 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -608,6 +608,12 @@ class RegionStoreManager : public StoreManager {
return getBinding(getRegionBindings(S), L, T);
}
+ /// Returns the value of the default binding of region \p BaseR
+ /// if and only if that is the unique binding in the cluster of \p BaseR.
+ /// \p BaseR must be a base region.
+ std::optional<SVal> getUniqueDefaultBinding(Store S,
+ const MemRegion *BaseR) 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
@@ -2605,9 +2611,42 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
return NewB;
}
+std::optional<SVal>
+RegionStoreManager::getUniqueDefaultBinding(Store S,
+ const MemRegion *BaseR) const {
+ assert(BaseR == BaseR->getBaseRegion() && "Expecting a base region");
+ const auto *Cluster = getRegionBindings(S).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 (LCV.getRegion()->getBaseRegion() == LCV.getRegion()) {
+ if (auto Val = getUniqueDefaultBinding(LCV.getStore(), LCV.getRegion())) {
+ return B.addBinding(BindingKey::Make(R, BindingKey::Default),
+ Val.value());
+ }
+ }
+
FieldVector Fields;
if (const CXXRecordDecl *Class = dyn_cast<CXXRecordDecl>(RD))
diff --git a/clang/test/Analysis/ctor-trivial-copy.cpp b/clang/test/Analysis/ctor-trivial-copy.cpp
index a1209c24136e20..ab623c1919e152 100644
--- a/clang/test/Analysis/ctor-trivial-copy.cpp
+++ b/clang/test/Analysis/ctor-trivial-copy.cpp
@@ -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": [
@@ -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: ]},
diff --git a/clang/test/Analysis/store-dump-orders.cpp b/clang/test/Analysis/store-dump-orders.cpp
index d99f581f00fe14..dbe93f1c5183a9 100644
--- a/clang/test/Analysis/store-dump-orders.cpp
+++ b/clang/test/Analysis/store-dump-orders.cpp
@@ -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" }
>From b56f29b8de7d8df1aca3bdd3db311cdd08f8614a Mon Sep 17 00:00:00 2001
From: Balazs Benics <benicsbalazs at gmail.com>
Date: Thu, 14 Nov 2024 15:46:09 +0100
Subject: [PATCH 2/2] Nest base region check into getUniqueDefaultBinding
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 26 +++++++++----------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index b54ba43ff42414..33f49fc666f99f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -608,11 +608,8 @@ class RegionStoreManager : public StoreManager {
return getBinding(getRegionBindings(S), L, T);
}
- /// Returns the value of the default binding of region \p BaseR
- /// if and only if that is the unique binding in the cluster of \p BaseR.
- /// \p BaseR must be a base region.
- std::optional<SVal> getUniqueDefaultBinding(Store S,
- const MemRegion *BaseR) const;
+ std::optional<SVal>
+ getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const;
std::optional<SVal> getDefaultBinding(Store S, const MemRegion *R) override {
RegionBindingsRef B = getRegionBindings(S);
@@ -2612,10 +2609,14 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
}
std::optional<SVal>
-RegionStoreManager::getUniqueDefaultBinding(Store S,
- const MemRegion *BaseR) const {
- assert(BaseR == BaseR->getBaseRegion() && "Expecting a base region");
- const auto *Cluster = getRegionBindings(S).lookup(BaseR);
+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;
@@ -2640,11 +2641,8 @@ std::optional<RegionBindingsRef> RegionStoreManager::tryBindSmallStruct(
// Direct [ 0..31]: Derived{Conj{}, w.width}
// Direct [32..63]: Derived{Conj{}, w.height}
// Instead, we should just bind that Conjured value instead.
- if (LCV.getRegion()->getBaseRegion() == LCV.getRegion()) {
- if (auto Val = getUniqueDefaultBinding(LCV.getStore(), LCV.getRegion())) {
- return B.addBinding(BindingKey::Make(R, BindingKey::Default),
- Val.value());
- }
+ if (auto Val = getUniqueDefaultBinding(LCV)) {
+ return B.addBinding(BindingKey::Make(R, BindingKey::Default), Val.value());
}
FieldVector Fields;
More information about the cfe-commits
mailing list