[analyzer] Avoid creating LazyCompoundVal when possible #116840
Conversation
In #115916 I allowed copying empty structs. Later in #115917 I changed how objects are copied, and basically when we would want to copy a struct (an LCV) of a single symbol (likely coming from an opaque fncall or invalidation), just directly bind that symbol instead of creating an LCV refering to the symbol. This was an optimization to skip a layer of indirection. Now, it turns out I should have apply the same logic in #115916. I should have just blindly create an LCV by calling `createLazyBinding()`, but rather check if I can apply the shortcut described in #115917 and only create the LCV if the shortcut doesn't apply. In this patch I check if there is a single default binding that the copy would refer to and if so, just return that symbol instead of creating an LCV. There shouldn't be any observable changes besides that we should have fewer LCVs. This change may surface bugs in checkers that were associating some metadata with entities in a wrong way. Notably, STLAlgorithmModeling and DebugIteratorModeling checkers would likely stop working after this change. I didn't investigate them deeply because they were broken even prior to this patch. Let me know if I should migrate these checkers to be just as bugged as they were prior to this patch - thus make the tests pass.
|
@necto Could you also review this one? |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesIn #115916 I allowed copying empty structs. Now, it turns out I should have apply the same logic in #115916. I should have just blindly create an LCV by calling In this patch I check if there is a single default binding that the copy would refer to and if so, just return that symbol instead of creating an LCV. There shouldn't be any observable changes besides that we should have fewer LCVs. This change may surface bugs in checkers that were associating some metadata with entities in a wrong way. Notably, STLAlgorithmModeling and DebugIteratorModeling checkers would likely stop working after this change. Full diff: https://github.com/llvm/llvm-project/pull/116840.diff 7 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index 46e294a1741cfe..ad45ab5757a5ac 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -608,6 +608,8 @@ class RegionStoreManager : public StoreManager {
return getBinding(getRegionBindings(S), L, T);
}
+ std::optional<SVal> getUniqueDefaultBinding(RegionBindingsConstRef B,
+ const TypedValueRegion *R) const;
std::optional<SVal>
getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const;
@@ -2349,6 +2351,11 @@ SVal RegionStoreManager::getBindingForStruct(RegionBindingsConstRef B,
// behavior doesn't depend on the struct layout.
// This way even an empty struct can carry taint, no matter if creduce drops
// the last field member or not.
+
+ // Try to avoid creating a LCV if it would anyways just refer to a single
+ // default binding.
+ if (std::optional<SVal> Val = getUniqueDefaultBinding(B, R))
+ return *Val;
return createLazyBinding(B, R);
}
@@ -2609,14 +2616,12 @@ RegionBindingsRef RegionStoreManager::bindVector(RegionBindingsConstRef B,
}
std::optional<SVal>
-RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
- const MemRegion *BaseR = LCV.getRegion();
-
- // We only handle base regions.
- if (BaseR != BaseR->getBaseRegion())
+RegionStoreManager::getUniqueDefaultBinding(RegionBindingsConstRef B,
+ const TypedValueRegion *R) const {
+ if (R != R->getBaseRegion())
return std::nullopt;
- const auto *Cluster = getRegionBindings(LCV.getStore()).lookup(BaseR);
+ const auto *Cluster = B.lookup(R);
if (!Cluster || !llvm::hasSingleElement(*Cluster))
return std::nullopt;
@@ -2624,6 +2629,12 @@ RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
return Key.isDirect() ? std::optional<SVal>{} : Value;
}
+std::optional<SVal>
+RegionStoreManager::getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const {
+ RegionBindingsConstRef B = getRegionBindings(LCV.getStore());
+ return getUniqueDefaultBinding(B, LCV.getRegion());
+}
+
std::optional<RegionBindingsRef> RegionStoreManager::tryBindSmallStruct(
RegionBindingsConstRef B, const TypedValueRegion *R, const RecordDecl *RD,
nonloc::LazyCompoundVal LCV) {
diff --git a/clang/test/Analysis/ctor-trivial-copy.cpp b/clang/test/Analysis/ctor-trivial-copy.cpp
index 41d0d97161bba1..45c8ca4c517762 100644
--- a/clang/test/Analysis/ctor-trivial-copy.cpp
+++ b/clang/test/Analysis/ctor-trivial-copy.cpp
@@ -3,8 +3,10 @@
void clang_analyzer_printState();
-template<typename T> void clang_analyzer_dump_lref(T&);
-template<typename T> void clang_analyzer_dump_val(T);
+template <typename T> void clang_analyzer_dump_lref(T& param);
+template <typename T> void clang_analyzer_dump_val(T param);
+template <typename T> void clang_analyzer_denote(T param, const char *name);
+template <typename T> void clang_analyzer_express(T param);
template <typename T> T conjure();
template <typename... Ts> void nop(const Ts &... args) {}
@@ -40,16 +42,17 @@ void test_assign_return() {
namespace trivial_struct_copy {
void _01_empty_structs() {
- clang_analyzer_dump_val(conjure<empty>()); // expected-warning {{lazyCompoundVal}}
+ clang_analyzer_dump_val(conjure<empty>()); // expected-warning {{conj_$}}
empty Empty = conjure<empty>();
empty Empty2 = Empty;
empty Empty3 = Empty2;
- // All of these should refer to the exact same LCV, because all of
+ // All of these should refer to the exact same symbol, because all of
// these trivial copies refer to the original conjured value.
// There were Unknown before:
- clang_analyzer_dump_val(Empty); // expected-warning {{lazyCompoundVal}}
- clang_analyzer_dump_val(Empty2); // expected-warning {{lazyCompoundVal}}
- clang_analyzer_dump_val(Empty3); // expected-warning {{lazyCompoundVal}}
+ clang_analyzer_denote(Empty, "$Empty");
+ clang_analyzer_express(Empty); // expected-warning {{$Empty}}
+ clang_analyzer_express(Empty2); // expected-warning {{$Empty}}
+ clang_analyzer_express(Empty3); // expected-warning {{$Empty}}
// We should have the same Conjured symbol for "Empty", "Empty2" and "Empty3".
clang_analyzer_printState();
@@ -75,15 +78,16 @@ void _01_empty_structs() {
}
void _02_structs_with_members() {
- clang_analyzer_dump_val(conjure<aggr>()); // expected-warning {{lazyCompoundVal}}
+ clang_analyzer_dump_val(conjure<aggr>()); // expected-warning {{conj_$}}
aggr Aggr = conjure<aggr>();
aggr Aggr2 = Aggr;
aggr Aggr3 = Aggr2;
- // All of these should refer to the exact same LCV, because all of
+ // All of these should refer to the exact same symbol, because all of
// these trivial copies refer to the original conjured value.
- clang_analyzer_dump_val(Aggr); // expected-warning {{lazyCompoundVal}}
- clang_analyzer_dump_val(Aggr2); // expected-warning {{lazyCompoundVal}}
- clang_analyzer_dump_val(Aggr3); // expected-warning {{lazyCompoundVal}}
+ clang_analyzer_denote(Aggr, "$Aggr");
+ clang_analyzer_express(Aggr); // expected-warning {{$Aggr}}
+ clang_analyzer_express(Aggr2); // expected-warning {{$Aggr}}
+ clang_analyzer_express(Aggr3); // expected-warning {{$Aggr}}
// We should have the same Conjured symbol for "Aggr", "Aggr2" and "Aggr3".
// We used to have Derived symbols for the individual fields that were
diff --git a/clang/test/Analysis/explain-svals.cpp b/clang/test/Analysis/explain-svals.cpp
index 33fce10c4e2b2c..d1615e6cc6c9aa 100644
--- a/clang/test/Analysis/explain-svals.cpp
+++ b/clang/test/Analysis/explain-svals.cpp
@@ -99,7 +99,7 @@ class C {
} // end of anonymous namespace
void test_6() {
- clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of 1st parameter of function 'clang_analyzer_explain\(\)'$}}}}
+ clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^symbol of type 'int' conjured at statement 'conjure_S\(\)'$}}}}
clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'int' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
}
diff --git a/clang/test/Analysis/iterator-modeling.cpp b/clang/test/Analysis/iterator-modeling.cpp
index f1538839d06c8e..78882da4431fd5 100644
--- a/clang/test/Analysis/iterator-modeling.cpp
+++ b/clang/test/Analysis/iterator-modeling.cpp
@@ -2035,6 +2035,7 @@ void print_state(std::vector<int> &V) {
// CHECK: "checker_messages": [
// CHECK: { "checker": "alpha.cplusplus.IteratorModeling", "messages": [
// CHECK-NEXT: "Iterator Positions :",
+ // CHECK-NEXT: "conj_$[[#]]{int, LC[[#]], S[[#]], #[[#]]} : Valid ; Container == SymRegion{reg_$[[#]]<std::vector<int> & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}",
// CHECK-NEXT: "i0 : Valid ; Container == SymRegion{reg_$[[#]]<std::vector<int> & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
// CHECK-NEXT: ]}
@@ -2045,6 +2046,7 @@ void print_state(std::vector<int> &V) {
// CHECK: "checker_messages": [
// CHECK: { "checker": "alpha.cplusplus.IteratorModeling", "messages": [
// CHECK-NEXT: "Iterator Positions :",
+ // CHECK-NEXT: "conj_$[[#]]{int, LC[[#]], S[[#]], #[[#]]} : Valid ; Container == SymRegion{reg_$[[#]]<std::vector<int> & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}",
// CHECK-NEXT: "i1 : Valid ; Container == SymRegion{reg_$[[#]]<std::vector<int> & V>} ; Offset == conj_$[[#]]{long, LC[[#]], S[[#]], #[[#]]}"
// CHECK-NEXT: ]}
diff --git a/clang/test/Analysis/stl-algorithm-modeling-aggressive-std-find-modeling.cpp b/clang/test/Analysis/stl-algorithm-modeling-aggressive-std-find-modeling.cpp
index 98301cf7274fc8..191af95cd2b9c2 100644
--- a/clang/test/Analysis/stl-algorithm-modeling-aggressive-std-find-modeling.cpp
+++ b/clang/test/Analysis/stl-algorithm-modeling-aggressive-std-find-modeling.cpp
@@ -4,6 +4,16 @@
// RUN: -analyzer-config alpha.cplusplus.STLAlgorithmModeling:AggressiveStdFindModeling=true\
// RUN: -verify
+// STLAlgorithmModeling and DebugIteratorModeling are probably bugged because
+// these tests started failing after we just directly copy the symbol
+// representing the value of a variable instead of creating a LazyCompoundVal
+// of that single conjured value.
+// In theory, it shouldn't matter if we eagerly copy the value that we would
+// "load" from the LCV once requested or just directly binding the backing symbol.
+// Yet, these tests fail, so there is likely messed up how/what the checker
+// metadata is associated with.
+// XFAIL: *
+
#include "Inputs/system-header-simulator-cxx.h"
void clang_analyzer_eval(bool);
diff --git a/clang/test/Analysis/stl-algorithm-modeling.cpp b/clang/test/Analysis/stl-algorithm-modeling.cpp
index 5549c24a8c220f..f7029c79b0942a 100644
--- a/clang/test/Analysis/stl-algorithm-modeling.cpp
+++ b/clang/test/Analysis/stl-algorithm-modeling.cpp
@@ -3,6 +3,16 @@
// RUN: -analyzer-config aggressive-binary-operation-simplification=true\
// RUN: -verify
+// STLAlgorithmModeling and DebugIteratorModeling are probably bugged because
+// these tests started failing after we just directly copy the symbol
+// representing the value of a variable instead of creating a LazyCompoundVal
+// of that single conjured value.
+// In theory, it shouldn't matter if we eagerly copy the value that we would
+// "load" from the LCV once requested or just directly binding the backing symbol.
+// Yet, these tests fail, so there is likely messed up how/what the checker
+// metadata is associated with.
+// XFAIL: *
+
#include "Inputs/system-header-simulator-cxx.h"
void clang_analyzer_eval(bool);
diff --git a/clang/test/Analysis/template-param-objects.cpp b/clang/test/Analysis/template-param-objects.cpp
index dde95fa62cb655..b065f8756d4d8f 100644
--- a/clang/test/Analysis/template-param-objects.cpp
+++ b/clang/test/Analysis/template-param-objects.cpp
@@ -11,7 +11,7 @@ bool operator ==(Box lhs, Box rhs) {
return lhs.value == rhs.value;
}
template <Box V> void dumps() {
- clang_analyzer_dump(V); // expected-warning {{lazyCompoundVal}}
+ clang_analyzer_dump(V); // expected-warning {{Unknown}}
clang_analyzer_dump(&V); // expected-warning {{Unknown}}
clang_analyzer_dump(V.value); // expected-warning {{Unknown}} FIXME: It should be '6 S32b'.
clang_analyzer_dump(&V.value); // expected-warning {{Unknown}}
|
|
Btw expect more patches about fixing LCVs and overall PostInitializer events and bindings. |
There was a problem hiding this comment.
The change LGTM and I'm happy to hear that you're improving the handling of compound values. I hope that these foundational improvements will help further development of checkers that deal with structured data. (Perhaps even the iterator checkers could be stabilized eventually...)
On our downstream branch this commit breaks a few tests, but I think that this just exposes faults of our internal-use checkers (which we'll update eventually).
By the way I think there is a typo in the PR description: the word "not" is missing from the location where I marked it in brackets:
I should [not] have just blindly create an LCV by calling createLazyBinding(), but rather check if I can apply the shortcut
(Also, "create" should be "created".)
Yes, this would be awesome. Unfortunately, I fix one thing and that uncovers something else, and continues.
Thanks for checking! Sorry for putting burden on you, and this is why I tried to motivate the change with a lengthy description. Hopefully they are also good source of learning.
Thanks for spotting! |
|
I held this patch off because during testing I saw some interesting-looking (seemingly) FPs. |
In #115916 I allowed copying empty structs.
Later in #115917 I changed how objects are copied, and basically when we would want to copy a struct (an LCV) of a single symbol (likely coming from an opaque fncall or invalidation), just directly bind that symbol instead of creating an LCV referring to the symbol. This was an optimization to skip a layer of indirection.
Now, it turns out I should have apply the same logic in #115916. I should not have just blindly created an LCV by calling
createLazyBinding(), but rather check if I can apply the shortcut described in #115917 and only create the LCV if the shortcut doesn't apply.In this patch I check if there is a single default binding that the copy would refer to and if so, just return that symbol instead of creating an LCV.
There shouldn't be any observable changes besides that we should have fewer LCVs. This change may surface bugs in checkers that were associating some metadata with entities in a wrong way. Notably, STLAlgorithmModeling and DebugIteratorModeling checkers would likely stop working after this change.
I didn't investigate them deeply because they were broken even prior to this patch. Let me know if I should migrate these checkers to be just as bugged as they were prior to this patch - thus make the tests pass.