fix[venom]: register self in global analyses cache fallback#5084
fix[venom]: register self in global analyses cache fallback#5084banteg wants to merge 2 commits into
Conversation
`IRAnalysesCache._ensure_global_analyses_cache` filled in a fresh `IRAnalysesCache` for every function missing from an existing global cache -- including `self.function` -- so the subsequent self-registration check was always false and `self` was never registered. global analyses would then operate on a parallel per-function cache, diverging from the cache owned by the active pass and risking stale analyses. register `self` first (matching the `global_cache is None` branch), then fill in fresh caches for the remaining functions. fixes vyperlang#5046
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31402cc9c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.function.ctx.global_analyses_cache = global_cache | ||
| return global_cache | ||
|
|
||
| global_cache.function_analyses_caches[self.function] = self |
There was a problem hiding this comment.
Avoid clobbering the registered function cache
When a global cache already exists, this unconditionally replaces the cache registered for self.function. If another IRAnalysesCache for the same function had already been installed by the pass pipeline, a temporary cache can now displace it; subsequent global analyses such as ReadonlyMemoryArgsGlobalAnalysis read per-function analyses through self.analyses_caches[fn].request_analysis(DFGAnalysis), so they can start using stale analyses from the temporary cache that the real pass cache never invalidates after IR mutations. This should only register self when the function has no cache entry, or otherwise avoid overwriting an existing authoritative cache for the same function.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — the unconditional overwrite traded one divergence hazard for another. Addressed in 33a528d:
IRAnalysesCache now tracks whether it was auto-created as a placeholder by _ensure_global_analyses_cache. The registration rule is: a requesting cache displaces a placeholder entry (or fills an absent one), but never a non-placeholder cache registered by a real consumer such as the pass pipeline.
"Register only when absent" alone wouldn't work here: both branches pre-fill caches for every function in the context, so a later real cache for one of those functions would find an entry already present (the placeholder) and the original #5046 bug — global analyses living in a parallel cache the active pass never invalidates — would persist. Distinguishing placeholders resolves both directions; test_existing_authoritative_cache_not_displaced pins the no-clobbering behavior alongside the original regression test.
What I did
Fixes #5046.
IRAnalysesCache._ensure_global_analyses_cachehad a fallback path (taken when a global analyses cache already exists) that never registeredself: the loop inserted a freshIRAnalysesCachefor every missing function includingself.function, making the trailing self-registration check dead code. Global analyses could then store per-function analyses in a parallel cache object, so the active pass's invalidations would not invalidate what later consumers read — a stale-analysis hazard.How I did it
Caches auto-created by
_ensure_global_analyses_cacheare now marked as placeholders. The registration rule: a requesting cache displaces a placeholder entry (or fills an absent one), but never a non-placeholder cache registered by a real consumer such as the pass pipeline — its consumers rely on its invalidations. Plain "register only when absent" would not fix #5046, since both branches pre-fill placeholder caches for every function in the context.How to verify it
New
tests/unit/compiler/venom/test_analyses_cache.py: the regression test makes a first global-analysis request from one function's cache (creating the global cache), then a second request from a freshIRAnalysesCache— the fallback path — and asserts the global cache maps the function to that same cache object. Fails on master with two distinct cache objects. (Note the single-request shape exercises the already-correctglobal_cache is Nonebranch, which is why the test needs the two-step setup.)Commit message
Description for the changelog
Fix: Venom global analyses cache registers the active function's analyses cache correctly on the fallback path, avoiding stale-analysis hazards outside the curated pass pipeline.
Cute Animal Picture