refactor Sparse Abstract Interpt#1812
Open
bjjwwang wants to merge 9 commits intoSVF-tools:masterfrom
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1812 +/- ##
=======================================
Coverage 64.19% 64.19%
=======================================
Files 249 250 +1
Lines 25037 25111 +74
Branches 4744 4743 -1
=======================================
+ Hits 16073 16121 +48
- Misses 8964 8990 +26
🚀 New features to boost your workflow:
|
yuleisui
reviewed
Apr 20, 2026
| static std::unique_ptr<AbstractInterpretation> instance = []() | ||
| -> std::unique_ptr<AbstractInterpretation> | ||
| { | ||
| if (Options::AESparsity() == AESparsity::SemiSparse) |
Collaborator
There was a problem hiding this comment.
Could you check the codebase and only keep this if condition here, and try to remove all other appearances of Options::AESparsity() == AESparsity::SemiSparse
…tation
Reorganise the AE class hierarchy so the factory targets concrete
mode-specific subclasses:
AbstractInterpretation (dense)
└── SparseAbstractInterpretation (sparse-shared cycle helpers)
├── SemiSparseAbstractInterpretation (current semi-sparse semantics)
└── FullSparseAbstractInterpretation (compile-only stub for now)
SparseAbstractInterpretation keeps its semi-sparse-shaped overrides
(pull cycle ValVars from def-sites, scatter widened/narrowed values
back). SemiSparseAbstractInterpretation inherits these unchanged.
FullSparseAbstractInterpretation is a TODO stub that compiles and
links — its overrides for ObjVar pull/scatter via SVFG land in a
follow-up PR (see doc/plan-full-sparse.md).
The factory in AbstractInterpretation::getAEInstance() now switches
on Options::AESparsity() and instantiates SemiSparse, FullSparse, or
the dense base accordingly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the four standalone files (Semi/FullSparseAbstractInterpretation .h/.cpp) introduced in the previous commit and inline the two concrete subclasses below SparseAbstractInterpretation in the same header. Both subclasses are tiny — empty body, defaulted ctor/dtor — so the extra TU pair is more noise than benefit. Factory in AbstractInterpretation::getAEInstance() unchanged structurally; only the include set shrinks back to one entry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the standalone AbstractStateManager.h/.cpp. The dense base now
lives next to AbstractInterpretation; the semi-sparse and full-sparse
managers sit beside SparseAbstractInterpretation:
AbstractStateManager (dense, AI files)
└── SemiSparseAbstractStateManager (SparseAI files)
└── FullSparseAbstractStateManager (adds SVFG)
Each AE subclass picks its manager via virtual createStateMgr(). The
per-cycle ValVar precompute is gated by a virtual needsCycleValVars(),
so dense and full-sparse skip it. Together this removes every
Options::AESparsity() check that lived inside the manager and inside
PreAnalysis::initCycleValVars; mode dispatch is now polymorphic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bjjwwang
force-pushed
the
subclass-sparse-ae
branch
from
bf9044e to
dfe3af0
Compare
bjjwwang
changed the title
yuleisui
reviewed
Apr 30, 2026
Comment on lines
+63
to
+66
| /// Hierarchy: | ||
| /// AbstractStateManager — dense (this file) | ||
| /// └── SemiSparseAbstractStateManager — semi-sparse (SparseAI file) | ||
| /// └── FullSparseAbstractStateManager — full-sparse (SparseAI file) |
Collaborator
There was a problem hiding this comment.
Are the names wrong here? We don't have SemiSparseAbstractStateManager or FullSparseAbstractStateManager
| /// | ||
| /// Each AbstractInterpretation subclass picks the matching manager via | ||
| /// the virtual factory `AbstractInterpretation::createStateMgr`. | ||
| class AbstractStateManager |
Collaborator
There was a problem hiding this comment.
Do we still need this class or could the methods be merged to AbstractInterpretation (it might be too large for this class if merged)?
| return abstractTrace.count(node) != 0; | ||
| } | ||
|
|
||
| const AbstractValue& AbstractStateManager::getAbstractValue(const ValVar* var, const ICFGNode* node) |
Collaborator
There was a problem hiding this comment.
getAbstractValue and hasAbstractValue look very complicated. Could we simplify them or wrap up them.
The current implementation look that there are a number of ad-hoc cases here.
Contributor
Author
There was a problem hiding this comment.
I split them. each class (dense/semi/full) has its get/hasAbstractValue.
AbstractState::joinWith now does a plain dense merge — it is a generic
lattice op and shouldn't peek at Options::AESparsity(). RelationSolver
keeps using it directly and gets stable semantics.
The mode-specific behaviour moves to a new virtual
AbstractStateManager::joinStates(dst, src):
* dense base — delegates to dst.joinWith(src)
* semi-sparse — snapshots dst's ValVars, performs the full join,
then restores them, leaving only ObjVars and the
freed-addr set merged
mergeStatesFromPredecessors routes its five join sites through the
manager. The strategy lives inside the subclass; AbstractState's
public API stays mode-agnostic.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The dense base used to inline four constant-class special cases, a def-site walk, a Call->Ret fallback, and a top-fallback write — the same chain duplicated across get and has and across dense and semi-sparse. Reviewer flagged it as ad-hoc. Dense base now does what its name suggests: read the var out of trace[node], or fall back to false / a default-constructed slot. All the resolution logic lives where it actually matters — in SemiSparseAbstractStateManager. FullSparseAbstractStateManager gets explicit get/has(ValVar*) overrides that assert(false), so the still-TODO full-sparse path fails loudly instead of inheriting semi-sparse semantics by accident. Dense ctest still passes — the def-site/Call->Ret fallback that the base used to carry was never reached in dense mode in practice; mergeStatesFromPredecessors and the per-stmt handlers populate the trace before any reader gets there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The two parallel hierarchies (AbstractInterpretation and
AbstractStateManager) were 1:1 coupled — every AI subclass had a
matching manager subclass, the createStateMgr() factory existed only
to wire them up, and AI's eight inline wrappers were pure pass-through
to the manager. Collapse the manager into the AI it serves.
AbstractInterpretation — owns abstractTrace + svfg
└── SparseAbstractInterpretation — semi-sparse cycle helpers + ValVar overrides
├── SemiSparseAbstractInterpretation
└── FullSparseAbstractInterpretation — initAuxState builds SVFG;
ValVar reads assert(false)
What disappears:
* AbstractStateManager class + Semi/FullSparse subclasses
* createStateMgr() virtual factory and svfStateMgr field
* Eight inline get/has/updateAbs* wrappers in AI
* Duplicate svfir storage
* `using AbstractStateManager::...` boilerplate from overload-hiding
What appears:
* abstractTrace, svfg fields directly on AI
* initAuxState() virtual hook, called from runOnModule once PTA is
available (FullSparse uses it to build the SVFG)
* AbsExtAPI's ctor takes AbstractInterpretation* (was AbstractStateManager*)
* AEDetector calls ae.<helper> directly (was ae.getStateMgr()->...)
The 100+ short-name uses (getAbsValue / hasAbsValue / ...) inside AI
and detectors are renamed to the manager's longer, explicit names
(getAbstractValue / hasAbstractValue / ...) so the codebase ends up
with a single canonical naming.
ctest: 586/586 ae_recursion + ae_semi_sparse + ae_overflow +
ae_nullptr_deref pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The factory now returns SemiSparseAbstractInterpretation / FullSparseAbstractInterpretation directly, not "a SparseAbstractInterpretation". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Linux gcc CI broke on std::setw — earlier the include arrived transitively through AbstractInterpretation.h, which I removed when the joinWith Options check went away. macOS clang's SDK still bundled iomanip via other system headers; gcc doesn't. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
todo: make Option::Semi-Sparse all in SpraseAbstractInterpreatation