[clang] [analyzer] Limit Store by region-store-binding-limit (PR #127602)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 18 01:54:14 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-static-analyzer-1
Author: Balázs Benics (balazs-benics-sonarsource)
<details>
<summary>Changes</summary>
In our test pool, the max entry point RT was improved by this change: 1'181 seconds (~19.7 minutes) -> 94 seconds (1.6 minutes)
BTW, the 1.6 minutes is still really bad. But a few orders of magnitude better than it was before.
This was the most servere RT edge-case as you can see from the numbers. There are are more known RT bottlenecks, such as:
- Large environment sizes, and `removeDead`. See more about the failed attempt on improving it at: https://discourse.llvm.org/t/unsuccessful-attempts-to-fix-a-slow-analysis-case-related-to-removedead-and-environment-size/84650
- Large chunk of time could be spend inside `assume`, to reach a fixed point. This is something we want to look into a bit later if we have time.
We have 3'075'607 entry points in our test set.
About 393'352 entry points ran longer than 1 second when measured.
To give a sense of the distribution, if we ignore the slowest 500 entry points, then the maximum entry point runs for about 14 seconds. These 500 slow entry points are in 332 translation units.
By this patch, out of the slowest 500 entry points, 72 entry points were improved by at least 10x after this change.
We measured no RT regression on the "usual" entry points.
(The dashed lines represent the maximum of their RT)
CPP-6092
---
Patch is 46.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127602.diff
9 Files Affected:
- (modified) clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def (+8)
- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h (+1-1)
- (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h (+9-1)
- (modified) clang/lib/StaticAnalyzer/Core/ProgramState.cpp (+14-4)
- (modified) clang/lib/StaticAnalyzer/Core/RegionStore.cpp (+149-61)
- (modified) clang/lib/StaticAnalyzer/Core/Store.cpp (+5-2)
- (modified) clang/test/Analysis/analyzer-config.c (+1)
- (modified) clang/test/Analysis/region-store.cpp (+334-2)
- (modified) clang/unittests/StaticAnalyzer/StoreTest.cpp (+4-3)
``````````diff
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
index a9b8d0753673b..f05c8724d583d 100644
--- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
+++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
@@ -483,6 +483,14 @@ ANALYZER_OPTION(
"behavior, set the option to 0.",
5)
+ANALYZER_OPTION(
+ unsigned, RegionStoreMaxBindingFanOut, "region-store-max-binding-fanout",
+ "This option limits how many sub-bindings a single binding operation can "
+ "scatter into. For example, binding an array would scatter into binding "
+ "each individual element. Setting this to zero means unlimited, but then "
+ "modelling large array initializers may take proportional time to their "
+ "size.", 100)
+
//===----------------------------------------------------------------------===//
// String analyzer options.
//===----------------------------------------------------------------------===//
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
index 20c446e33ef9a..9fd07ce47175c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -659,13 +659,13 @@ class ExprEngine {
SVal Loc, SVal Val,
const LocationContext *LCtx);
+public:
/// A simple wrapper when you only need to notify checkers of pointer-escape
/// of some values.
ProgramStateRef escapeValues(ProgramStateRef State, ArrayRef<SVal> Vs,
PointerEscapeKind K,
const CallEvent *Call = nullptr) const;
-public:
// FIXME: 'tag' should be removed, and a LocationContext should be used
// instead.
// FIXME: Comment on the meaning of the arguments, when 'St' may not
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
index 332855a3c9c45..ebf00d49b6cc8 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -50,6 +50,14 @@ class SymbolReaper;
using InvalidatedSymbols = llvm::DenseSet<SymbolRef>;
+struct BindResult {
+ StoreRef ResultingStore;
+
+ // If during the bind operation we exhaust the allowed binding budget, we set
+ // this to the beginning of the escaped part of the region.
+ llvm::SmallVector<SVal, 0> FailedToBindValues;
+};
+
class StoreManager {
protected:
SValBuilder &svalBuilder;
@@ -105,7 +113,7 @@ class StoreManager {
/// \return A StoreRef object that contains the same
/// bindings as \c store with the addition of having the value specified
/// by \c val bound to the location given for \c loc.
- virtual StoreRef Bind(Store store, Loc loc, SVal val) = 0;
+ virtual BindResult Bind(Store store, Loc loc, SVal val) = 0;
/// Return a store with the specified value bound to all sub-regions of the
/// region. The region must not have previous bindings. If you need to
diff --git a/clang/lib/StaticAnalyzer/Core/ProgramState.cpp b/clang/lib/StaticAnalyzer/Core/ProgramState.cpp
index 34ab2388cbd2f..325b44c9cb05c 100644
--- a/clang/lib/StaticAnalyzer/Core/ProgramState.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -116,13 +116,23 @@ ProgramStateRef ProgramState::bindLoc(Loc LV,
const LocationContext *LCtx,
bool notifyChanges) const {
ProgramStateManager &Mgr = getStateManager();
- ProgramStateRef newState = makeWithStore(Mgr.StoreMgr->Bind(getStore(),
- LV, V));
+ ExprEngine &Eng = Mgr.getOwningEngine();
+ BindResult BindRes = Mgr.StoreMgr->Bind(getStore(), LV, V);
+ ProgramStateRef State = makeWithStore(BindRes.ResultingStore);
const MemRegion *MR = LV.getAsRegion();
+
+ // We must always notify the checkers for failing binds because otherwise they
+ // may keep stale traits for these symbols.
+ // Eg., Malloc checker may report leaks if we failed to bind that symbol.
+ if (!BindRes.FailedToBindValues.empty()) {
+ State =
+ Eng.escapeValues(State, BindRes.FailedToBindValues, PSK_EscapeOnBind);
+ }
+
if (MR && notifyChanges)
- return Mgr.getOwningEngine().processRegionChange(newState, MR, LCtx);
+ return Eng.processRegionChange(State, MR, LCtx);
- return newState;
+ return State;
}
ProgramStateRef
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index d01b6ae55f611..ee821f9b19e73 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -164,6 +164,7 @@ namespace {
class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
ClusterBindings> {
ClusterBindings::Factory *CBFactory;
+ SmallVectorImpl<SVal> *EscapedValuesDuringBind;
// This flag indicates whether the current bindings are within the analysis
// that has started from main(). It affects how we perform loads from
@@ -176,31 +177,59 @@ class RegionBindingsRef : public llvm::ImmutableMapRef<const MemRegion *,
// however that would have made the manager needlessly stateful.
bool IsMainAnalysis;
+ unsigned BindingsLeft;
+
public:
+ unsigned bindingsLeft() const { return BindingsLeft; }
+
+ bool hasExhaustedBindingLimit() const { return BindingsLeft == 0; }
+
+ RegionBindingsRef escapeValue(SVal V) const {
+ assert(EscapedValuesDuringBind);
+ EscapedValuesDuringBind->push_back(V);
+ return *this;
+ }
+ RegionBindingsRef escapeValues(nonloc::CompoundVal::iterator Begin,
+ nonloc::CompoundVal::iterator End) const {
+ for (SVal V : llvm::make_range(Begin, End))
+ escapeValue(V);
+ return *this;
+ }
+
typedef llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>
ParentTy;
RegionBindingsRef(ClusterBindings::Factory &CBFactory,
+ SmallVectorImpl<SVal> *EscapedValuesDuringBind,
const RegionBindings::TreeTy *T,
- RegionBindings::TreeTy::Factory *F,
- bool IsMainAnalysis)
- : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(T, F),
- CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
-
- RegionBindingsRef(const ParentTy &P,
- ClusterBindings::Factory &CBFactory,
- bool IsMainAnalysis)
- : llvm::ImmutableMapRef<const MemRegion *, ClusterBindings>(P),
- CBFactory(&CBFactory), IsMainAnalysis(IsMainAnalysis) {}
+ RegionBindings::TreeTy::Factory *F, bool IsMainAnalysis,
+ unsigned BindingsLeft)
+ : RegionBindingsRef(ParentTy(T, F), CBFactory, EscapedValuesDuringBind,
+ IsMainAnalysis, BindingsLeft) {}
+
+ RegionBindingsRef(const ParentTy &P, ClusterBindings::Factory &CBFactory,
+ SmallVectorImpl<SVal> *EscapedValuesDuringBind,
+ bool IsMainAnalysis, unsigned BindingsLeft)
+ : ParentTy(P), CBFactory(&CBFactory),
+ EscapedValuesDuringBind(EscapedValuesDuringBind),
+ IsMainAnalysis(IsMainAnalysis), BindingsLeft(BindingsLeft) {}
+
+ RegionBindingsRef add(key_type_ref K, data_type_ref D,
+ unsigned NewBindingsLeft) const {
+ return RegionBindingsRef(static_cast<const ParentTy *>(this)->add(K, D),
+ *CBFactory, EscapedValuesDuringBind,
+ IsMainAnalysis, NewBindingsLeft);
+ }
RegionBindingsRef add(key_type_ref K, data_type_ref D) const {
- return RegionBindingsRef(static_cast<const ParentTy *>(this)->add(K, D),
- *CBFactory, IsMainAnalysis);
+ unsigned NewBindingsLeft = BindingsLeft ? BindingsLeft - 1 : BindingsLeft;
+ return add(K, D, NewBindingsLeft);
}
RegionBindingsRef remove(key_type_ref K) const {
return RegionBindingsRef(static_cast<const ParentTy *>(this)->remove(K),
- *CBFactory, IsMainAnalysis);
+ *CBFactory, EscapedValuesDuringBind,
+ IsMainAnalysis, BindingsLeft);
}
RegionBindingsRef addBinding(BindingKey K, SVal V) const;
@@ -345,14 +374,21 @@ RegionBindingsRef::getDefaultBinding(const MemRegion *R) const {
}
RegionBindingsRef RegionBindingsRef::addBinding(BindingKey K, SVal V) const {
+ // If we are about to exhaust the binding limit, highjack this bind call for
+ // the default binding.
+ if (BindingsLeft == 1) {
+ escapeValue(V);
+ K = BindingKey::Make(K.getRegion(), BindingKey::Default);
+ V = UnknownVal();
+ }
+
const MemRegion *Base = K.getBaseRegion();
const ClusterBindings *ExistingCluster = lookup(Base);
ClusterBindings Cluster =
(ExistingCluster ? *ExistingCluster : CBFactory->getEmptyMap());
- ClusterBindings NewCluster = CBFactory->add(Cluster, K, V);
- return add(Base, NewCluster);
+ return add(Base, CBFactory->add(Cluster, K, V));
}
@@ -417,7 +453,7 @@ class RegionStoreManager : public StoreManager {
///
/// This is controlled by 'region-store-small-struct-limit' option.
/// To disable all small-struct-dependent behavior, set the option to "0".
- unsigned SmallStructLimit;
+ const unsigned SmallStructLimit;
/// The largest number of element an array can have and still be
/// considered "small".
@@ -427,7 +463,13 @@ class RegionStoreManager : public StoreManager {
///
/// This is controlled by 'region-store-small-struct-limit' option.
/// To disable all small-struct-dependent behavior, set the option to "0".
- unsigned SmallArrayLimit;
+ const unsigned SmallArrayLimit;
+
+ /// The number of bindings a single bind operation can scatter into.
+ /// For example, binding the initializer-list of an array would recurse and
+ /// bind all the individual array elements, potentially causing scalability
+ /// issues.
+ const unsigned RegionStoreMaxBindingFanOut;
/// A helper used to populate the work list with the given set of
/// regions.
@@ -435,15 +477,21 @@ class RegionStoreManager : public StoreManager {
ArrayRef<SVal> Values,
InvalidatedRegions *TopLevelRegions);
+ const AnalyzerOptions &getOptions() {
+ return StateMgr.getOwningEngine().getAnalysisManager().options;
+ }
+
public:
RegionStoreManager(ProgramStateManager &mgr)
: StoreManager(mgr), RBFactory(mgr.getAllocator()),
- CBFactory(mgr.getAllocator()), SmallStructLimit(0), SmallArrayLimit(0) {
- ExprEngine &Eng = StateMgr.getOwningEngine();
- AnalyzerOptions &Options = Eng.getAnalysisManager().options;
- SmallStructLimit = Options.RegionStoreSmallStructLimit;
- SmallArrayLimit = Options.RegionStoreSmallArrayLimit;
- }
+ CBFactory(mgr.getAllocator()),
+ SmallStructLimit(getOptions().RegionStoreSmallStructLimit),
+ SmallArrayLimit(getOptions().RegionStoreSmallArrayLimit),
+ RegionStoreMaxBindingFanOut(
+ getOptions().RegionStoreMaxBindingFanOut == 0
+ ? -1U
+ : getOptions().RegionStoreMaxBindingFanOut +
+ /*for the default binding*/ 1) {}
/// setImplicitDefaultValue - Set the default binding for the provided
/// MemRegion to the value implicitly defined for compound literals when
@@ -465,9 +513,13 @@ class RegionStoreManager : public StoreManager {
bool IsMainAnalysis = false;
if (const auto *FD = dyn_cast<FunctionDecl>(InitLoc->getDecl()))
IsMainAnalysis = FD->isMain() && !Ctx.getLangOpts().CPlusPlus;
- return StoreRef(RegionBindingsRef(
- RegionBindingsRef::ParentTy(RBFactory.getEmptyMap(), RBFactory),
- CBFactory, IsMainAnalysis).asStore(), *this);
+ return StoreRef(
+ RegionBindingsRef(
+ RegionBindingsRef::ParentTy(RBFactory.getEmptyMap(), RBFactory),
+ CBFactory, /*EscapedValuesDuringBind=*/nullptr, IsMainAnalysis,
+ RegionStoreMaxBindingFanOut)
+ .asStore(),
+ *this);
}
//===-------------------------------------------------------------------===//
@@ -502,9 +554,13 @@ class RegionStoreManager : public StoreManager {
QualType ElemT);
public: // Part of public interface to class.
-
- StoreRef Bind(Store store, Loc LV, SVal V) override {
- return StoreRef(bind(getRegionBindings(store), LV, V).asStore(), *this);
+ BindResult Bind(Store store, Loc LV, SVal V) override {
+ llvm::SmallVector<SVal, 0> EscapedValuesDuringBind;
+ return BindResult{
+ StoreRef(bind(getRegionBindings(store, &EscapedValuesDuringBind), LV, V)
+ .asStore(),
+ *this),
+ EscapedValuesDuringBind};
}
RegionBindingsRef bind(RegionBindingsConstRef B, Loc LV, SVal V);
@@ -513,7 +569,7 @@ class RegionStoreManager : public StoreManager {
// a default value.
StoreRef BindDefaultInitial(Store store, const MemRegion *R,
SVal V) override {
- RegionBindingsRef B = getRegionBindings(store);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(store);
// Use other APIs when you have to wipe the region that was initialized
// earlier.
assert(!(B.getDefaultBinding(R) || B.getDirectBinding(R)) &&
@@ -538,7 +594,7 @@ class RegionStoreManager : public StoreManager {
if (BR->getDecl()->isEmpty())
return StoreRef(store, *this);
- RegionBindingsRef B = getRegionBindings(store);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(store);
SVal V = svalBuilder.makeZeroVal(Ctx.CharTy);
B = removeSubRegionBindings(B, cast<SubRegion>(R));
B = B.addBinding(BindingKey::Make(R, BindingKey::Default), V);
@@ -587,14 +643,14 @@ class RegionStoreManager : public StoreManager {
StoreRef killBinding(Store ST, Loc L) override;
void incrementReferenceCount(Store store) override {
- getRegionBindings(store).manualRetain();
+ getRegionBindingsWithUnboundedLimit(store).manualRetain();
}
/// If the StoreManager supports it, decrement the reference count of
/// the specified Store object. If the reference count hits 0, the memory
/// associated with the object is recycled.
void decrementReferenceCount(Store store) override {
- getRegionBindings(store).manualRelease();
+ getRegionBindingsWithUnboundedLimit(store).manualRelease();
}
bool includedInBindings(Store store, const MemRegion *region) const override;
@@ -613,7 +669,7 @@ class RegionStoreManager : public StoreManager {
/// else
/// return symbolic
SVal getBinding(Store S, Loc L, QualType T) override {
- return getBinding(getRegionBindings(S), L, T);
+ return getBinding(getRegionBindingsWithUnboundedLimit(S), L, T);
}
std::optional<SVal> getUniqueDefaultBinding(RegionBindingsConstRef B,
@@ -622,7 +678,7 @@ class RegionStoreManager : public StoreManager {
getUniqueDefaultBinding(nonloc::LazyCompoundVal LCV) const;
std::optional<SVal> getDefaultBinding(Store S, const MemRegion *R) override {
- RegionBindingsRef B = getRegionBindings(S);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(S);
// Default bindings are always applied over a base region so look up the
// base region's default binding, otherwise the lookup will fail when R
// is at an offset from R->getBaseRegion().
@@ -700,21 +756,23 @@ class RegionStoreManager : public StoreManager {
// Utility methods.
//===------------------------------------------------------------------===//
- RegionBindingsRef getRegionBindings(Store store) const {
- llvm::PointerIntPair<Store, 1, bool> Ptr;
- Ptr.setFromOpaqueValue(const_cast<void *>(store));
- return RegionBindingsRef(
- CBFactory,
- static_cast<const RegionBindings::TreeTy *>(Ptr.getPointer()),
- RBFactory.getTreeFactory(),
- Ptr.getInt());
+ RegionBindingsRef
+ getRegionBindings(Store store,
+ SmallVectorImpl<SVal> *EscapedValuesDuringBind) const {
+ return getRegionBindingsImpl(store, EscapedValuesDuringBind,
+ /*BindingsLeft=*/RegionStoreMaxBindingFanOut);
+ }
+
+ RegionBindingsRef getRegionBindingsWithUnboundedLimit(Store store) const {
+ return getRegionBindingsImpl(store, /*EscapedValuesDuringBind=*/nullptr,
+ /*BindingsLeft=*/-1U);
}
void printJson(raw_ostream &Out, Store S, const char *NL = "\n",
unsigned int Space = 0, bool IsDot = false) const override;
void iterBindings(Store store, BindingsHandler& f) override {
- RegionBindingsRef B = getRegionBindings(store);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(store);
for (const auto &[Region, Cluster] : B) {
for (const auto &[Key, Value] : Cluster) {
if (!Key.isDirect())
@@ -727,6 +785,19 @@ class RegionStoreManager : public StoreManager {
}
}
}
+
+private:
+ RegionBindingsRef
+ getRegionBindingsImpl(Store store,
+ SmallVectorImpl<SVal> *EscapedValuesDuringBind,
+ unsigned BindingsLeft) const {
+ llvm::PointerIntPair<Store, 1, bool> Ptr;
+ Ptr.setFromOpaqueValue(const_cast<void *>(store));
+ return RegionBindingsRef(
+ CBFactory, EscapedValuesDuringBind,
+ static_cast<const RegionBindings::TreeTy *>(Ptr.getPointer()),
+ RBFactory.getTreeFactory(), Ptr.getInt(), BindingsLeft);
+ }
};
} // end anonymous namespace
@@ -852,7 +923,7 @@ class ClusterAnalysis {
bool RegionStoreManager::scanReachableSymbols(Store S, const MemRegion *R,
ScanReachableSymbols &Callbacks) {
assert(R == R->getBaseRegion() && "Should only be called for base regions");
- RegionBindingsRef B = getRegionBindings(S);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(S);
const ClusterBindings *Cluster = B.lookup(R);
if (!Cluster)
@@ -1038,7 +1109,9 @@ RegionStoreManager::removeSubRegionBindings(RegionBindingsConstRef B,
if (Result.isEmpty())
return B.remove(ClusterHead);
- return B.add(ClusterHead, Result.asImmutableMap());
+ // Make this "add" free by using the old "BindingsLeft".
+ return B.add(ClusterHead, Result.asImmutableMap(),
+ /*BindingsLeft=*/B.bindingsLeft());
}
namespace {
@@ -1375,7 +1448,7 @@ StoreRef RegionStoreManager::invalidateRegions(
GlobalsFilter = GFK_None;
}
- RegionBindingsRef B = getRegionBindings(store);
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(store);
InvalidateRegionsWorker W(*this, StateMgr, B, S, Count, LCtx, IS, ITraits,
Invalidated, GlobalsFilter);
@@ -2136,8 +2209,9 @@ RegionStoreManager::getBindingForFieldOrElementCommon(RegionBindingsConstRef B,
const SubRegion *lazyBindingRegion = nullptr;
std::tie(lazyBindingStore, lazyBindingRegion) = findLazyBinding(B, R, R);
if (lazyBindingRegion)
- return getLazyBinding(lazyBindingRegion,
- getRegionBindings(lazyBindingStore));
+ return getLazyBinding(
+ lazyBindingRegion,
+ getRegionBindingsWithUnboundedLimit(lazyBindingStore));
// Record whether or not we see a symbolic index. That can completely
// be out of scope of our lookup.
@@ -2314,7 +2388,7 @@ RegionStoreManager::getInterestingValues(nonloc::LazyCompoundVal LCV) {
SValListTy List;
const SubRegion *LazyR = LCV.getRegion();
- RegionBindingsRef B = getRegionBindings(LCV.getStore());
+ RegionBindingsRef B = getRegionBindingsWithUnboundedLimit(LCV.getStore());
// If this region had /no/ bindings at the time, there are no interesting
// values to return.
@@ -2377,7 +2451,7 @@ SVal RegionStoreManager::getBindingForArray(RegionBindingsConstRef B,
bool RegionStoreManager::includedInBindings(Store store,
const MemRegion *region) const {
- RegionBindingsRef B = getRegionBindings(store)...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/127602
More information about the cfe-commits
mailing list