https://github.com/NagyDonat commented:
I'm really happy that you managed to track down and fix these slow corner
cases, and overall I like the code changes, but I 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... :wink:).
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 -- and I'll discuss in the rest of this
"main" review comment.
If I understand correctly, your approach does the following steps:
1. The constructor of `RegionManagerStore` (usually) initializes
`RegionStoreMaxBindingFanOut` to `getOptions().RegionStoreMaxBindingFanOut +
1` -- I think this off-by-one difference between identically named variables is
a serious drawback of your approach.
2. `RegionManagerStore::getRegionBindings()` uses this adjusted fan-out value
to construct the `RegionBindingsRef` object and initialize its field
`BindingsLeft`.
3. `BindingsLeft` is decremented by the `RegionBindingsRef::add` method (which
adds _a cluster_).
- if `BindingsLeft` is 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 `removeSubRegionBindings` uses the
3-argument form of the `add` method to avoid decrementing the `BindingsLeft`
counter.
4. If `BindingsLeft == 1`, then `RegionBindingsRef::addBinding` hijacks the
call and adds a default binding instead of the one that's specified by its
argument.
5. If `BindingsLeft == 0`, then (as far as I see) early return branches with
`if (B.hasExhaustedBindingLimit())` stop most (all?) code paths that would add
bindings or clusters.
6. On these early return branches `escapeValue()` or `escapeValues()` calls
ensure that the values that we ignored are marked as escaped.
In this model there are three phases of the binding creation process:
1. `BindingsLeft > 1`, bindings are created normally
2. `BindingsLeft == 1`, intermediate state:
- `add` works, can add clusters and can decrement `BindingsLeft` to zero
- if `BindingsLeft` is decremented to zero here, then I don't think that
the "add a default binding" step happens
- `addBindings` adds a default binding (which decrements `BindingsLeft`)
and records that the `SVal` specified in its argument has escaped
3. `BindingsLeft == 0`, exhausted state:
- `hasExhaustedBindingLimit()` early returns prevent most (all?) addition
of bindings
- but e.g. the three-argument `add` within `removeSubRegionBindings`
still 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:
1. The constructor of `RegionManagerStore` always initializes
`RegionStoreMaxBindingFanOut` to `getOptions().RegionStoreMaxBindingFanOut` --
to avoid discrepancies and confusion between them.
2. If this fan-out value is nonzero, then
`RegionManagerStore::getRegionBindings()` uses it directly to initialize
`RegionBindingsRef::BindingsLeft` (i.e. it does not add one).
- If the fan-out value is zero (which means unlimited), `BindingsLeft` may
be initialized to `-1u`.
- Alternatively, perhaps it would be more modern (but slightly more
verbose) to say that `BindingsLeft` is an `std::optional<unsigned>` and it's
initialized to `std:nullopt` when there is no limit (i.e. when using
`getRegionBindingsWithUnboundedLimit` or when `RegionStoreMaxBindingFanOut ==
0`).
3. **(unchanged)** `BindingsLeft` is decremented by the
`RegionBindingsRef::add` method (which adds _a cluster_).
- if `BindingsLeft` is 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 `removeSubRegionBindings` uses the
3-argument form of the `add` method to avoid decrementing the `BindingsLeft`
counter.
4. _There is no highjacking logic and `BindingsLeft == 1` has no special
meaning._
5. **(unchanged)** If `BindingsLeft == 0`, then `B.hasExhaustedBindingLimit()`
is true, which triggers early return on most (all?) code paths that would add
bindings or clusters.
6. `escapeValue` and `escapeValues` are 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
`RecordBindingsRef` stores incomplete information.
WDYT about this alternative architecture?
https://github.com/llvm/llvm-project/pull/127602
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
