[analyzer] Limit Store by region-store-binding-limit #127602
Conversation
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. CPP-6092
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Benics (balazs-benics-sonarsource) ChangesIn 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:
We have 3'075'607 entry points in our test set. 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.
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:
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]
|
|
(I'm writing a review right now, please wait a bit -- at most a few hours -- before merging.) |
No worries. I wouldnt have merged tgis without your approval too. |
There was a problem hiding this comment.
I'm really happy that you managed to track down and fix these slow corner cases, and overall I like the code changes, but there were a few places where the code was difficult to understand for me (although this may be also related to the fact that I didn't sleep enough last night... 😉).
I added a few inline comments to mark some confusing parts. Moreover, I also felt that it would be possible to implement the exhaustion "event" more elegantly with a different approach:
If I understand correctly, your approach does the following steps:
- The constructor of
RegionManagerStore(usually) initializesRegionStoreMaxBindingFanOuttogetOptions().RegionStoreMaxBindingFanOut + 1-- I think this off-by-one difference between identically named variables is a serious drawback of your approach. RegionManagerStore::getRegionBindings()uses this adjusted fan-out value to construct theRegionBindingsRefobject and initialize its fieldBindingsLeft.BindingsLeftis decremented by theRegionBindingsRef::addmethod (which adds a cluster).- if
BindingsLeftis already zero, it's not decremented (so it won't overflow)- is this branch actually used?
- if not, then it should be replaced by an assertion
- there is an exceptional case where
removeSubRegionBindingsuses the 3-argument form of theaddmethod to avoid decrementing theBindingsLeftcounter.
- if
- If
BindingsLeft == 1, thenRegionBindingsRef::addBindinghijacks the call and adds a default binding instead of the one that's specified by its argument. - If
BindingsLeft == 0, then (as far as I see) early return branches withif (B.hasExhaustedBindingLimit())stop most (all?) code paths that would add bindings or clusters. - On these early return branches
escapeValue()orescapeValues()calls ensure that the values that we ignored are marked as escaped.
In this model there are three phases of the binding creation process:
BindingsLeft > 1, bindings are created normallyBindingsLeft == 1, intermediate state:addworks, can add clusters and can decrementBindingsLeftto zero- if
BindingsLeftis decremented to zero here, then I don't think that the "add a default binding" step happens
- if
addBindingsadds a default binding (which decrementsBindingsLeft) and records that theSValspecified in its argument has escaped
BindingsLeft == 0, exhausted state:hasExhaustedBindingLimit()early returns prevent most (all?) addition of bindings- but e.g. the three-argument
addwithinremoveSubRegionBindingsstill works - and perhaps there are some other code paths...
(I spent hours on trying to understand this, but I'm still not completely confident that I got every corner case right.) I didn't find anything that is outright buggy, but sometimes I felt that the code tries to preserve an invariant (e.g. "default binding is always added just before exhaustion"), but there is another branch where I don't see why is it preserved.
To simplify the picture a bit, I'd suggest the following alternative approach:
- The constructor of
RegionManagerStorealways initializesRegionStoreMaxBindingFanOuttogetOptions().RegionStoreMaxBindingFanOut-- to avoid discrepancies and confusion between them. - If this fan-out value is nonzero, then
RegionManagerStore::getRegionBindings()uses it directly to initializeRegionBindingsRef::BindingsLeft(i.e. it does not add one).- If the fan-out value is zero (which means unlimited),
BindingsLeftmay be initialized to-1u. - Alternatively, perhaps it would be more modern (but slightly more verbose) to say that
BindingsLeftis anstd::optional<unsigned>and it's initialized tostd:nulloptwhen there is no limit (i.e. when usinggetRegionBindingsWithUnboundedLimitor whenRegionStoreMaxBindingFanOut == 0).
- If the fan-out value is zero (which means unlimited),
- (unchanged)
BindingsLeftis decremented by theRegionBindingsRef::addmethod (which adds a cluster).- if
BindingsLeftis already zero, it's not decremented (so it won't overflow)- is this branch actually used? -- if not, then it should be replaced by an assertion
- there is an exceptional case where
removeSubRegionBindingsuses the 3-argument form of theaddmethod to avoid decrementing theBindingsLeftcounter.
- if
- There is no highjacking logic and
BindingsLeft == 1has no special meaning. - (unchanged) If
BindingsLeft == 0, thenB.hasExhaustedBindingLimit()is true, which triggers early return on most (all?) code paths that would add bindings or clusters. escapeValueandescapeValuesare replaced by more general functions that- still mark the "leftover" values as escaped
- but also add a default binding (with unknown value) to mark that the
RecordBindingsRefstores incomplete information.
WDYT about this alternative architecture?
|
As I thought a bit more about the reorganization that I suggested, I realized that it can be summarized as we should synchronize adding the default Of course there is some asymmetry in that |
…nify that it should be avoided
|
Thanks for your long review. I'm sorry if the poor code quality hindered the comprehension. I'm still working through your review, but I wanted to post a quick update because I think the renamings in place now may make refining your stance easier. In short, I decided to put a strong type in place to track and enforce the bind limit. Now, we should have greater confidence of that nothing misses the checks. However, this comes at the cost of polluting the other APIs, like Now, I'll get back to the rest of your comments and respond eventually. Thanks! |
There was a problem hiding this comment.
I'm sorry if the poor code quality hindered the comprehension.
I wouldn't say "poor code quality" -- the code was difficult to understand, but mostly due to the inherent complexity of the logic that's being implemented here.
In short, I decided to put a strong type in place to track and enforce the bind limit.
I agree with this direction.
AFAIK it's not possible (or rather would be ugly) to tie "adding the default unknown binding" to For example, while binding a compound val like this: If we didn't add the default binding Unknown earlier, at this point it would be difficult to recover the memory region to the specific element from which the escapes happened, aka. the memregion of the access pattern to I think I'm finished, I looked at everything again, and I still believe it's as good as it gets. |
|
I tried to write down my rough ideas as a concrete commit, but I don't have enough mental acuity for it today. I'll try to make another attempt on Monday -- but if you wish to close this commit, then it's also OK if I do this refactoring in a separate follow-up commit. |
There was a problem hiding this comment.
Anyway, let's just merge this as it is now.
The code is basically OK, I still don't have the brainpower to hold all the details in my mind (kudos for the fact that you managed to put this together) and if I'll catch some divine inspiration in the future, I can still refactor this as a follow-up commit.
Thank you. I'll merge this later today. |
balazs-benics-sonarsource
deleted the
bb/upstreaming-cpp-6092-limit-store
branch
|
@balazs-benics-sonarsource Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
Hello @balazs-benics-sonarsource The following starts crashing with this patch: |
Thank you for the heads up @mikaelholmen. I'll switch to it ASAP. I'd expect the fix later today. |
|
Confirmed crash, https://compiler-explorer.com/z/fzoqP36xq |
Basically, we may leave the loop because if exhaust the fields, array elements or other subobjects to initialize. In that case, the Bindings may be in an exhausted state, thus no further addBinding calls are allowed. Let's harden the code by sprinkling some early exists in the recursive dispatcher functions. And to actually fix the issue, I added a check guarding the single unguarded addBinding right after a loop I mentioned. Fixes llvm#129211
Basically, we may leave the loop because if exhaust the fields, array elements or other subobjects to initialize. In that case, the Bindings may be in an exhausted state, thus no further addBinding calls are allowed. Let's harden the code by sprinkling some early exists in the recursive dispatcher functions. And to actually fix the issue, I added a check guarding the single unguarded addBinding right after a loop I mentioned. Fixes #129211
Basically, we may leave the loop because if exhaust the fields, array elements or other subobjects to initialize. In that case, the Bindings may be in an exhausted state, thus no further addBinding calls are allowed. Let's harden the code by sprinkling some early exists in the recursive dispatcher functions. And to actually fix the issue, I added a check guarding the single unguarded addBinding right after a loop I mentioned. Fixes llvm#129211
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/84650Large 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