Go: Avoid combinatorial explosion in mostRecentSideEffect when there are multiple entry points#21753
Open
hvitved wants to merge 1 commit intogithub:mainfrom
Open
Go: Avoid combinatorial explosion in mostRecentSideEffect when there are multiple entry points#21753hvitved wants to merge 1 commit intogithub:mainfrom
mostRecentSideEffect when there are multiple entry points#21753hvitved wants to merge 1 commit intogithub:mainfrom
Conversation
…e are multiple entry points
hvitved
force-pushed
the
go/most-recent-side-effect-multi-entry
branch
from
84d3f61 to
8e26fa1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Go global value numbering’s side-effect dominator computation to avoid a combinatorial blow-up when multiple CFG entry points exist (e.g., due to TRAP deduplication), by separating computations per entry point and reintroducing uniqueness at the call sites that require it.
Changes:
- Parameterize immediate-dominator computation (
iDomEffect) andmostRecentSideEffectby a specific CFG entry node to prevent cross-entry mixing. - Add a cached
mostRecentSideEffectUnique(node)wrapper that enforces a single result viaunique(...). - Thread the computed dominator through analyzable expression predicates (field reads, incomplete-SSA variable reads, index reads, pointer dereferences) using
mostRecentSideEffectUnique.
Show a summary per file
| File | Description |
|---|---|
| go/ql/lib/semmle/go/dataflow/GlobalValueNumbering.qll | Refactors side-effect dominator logic to be entry-specific and introduces a cached uniqueness wrapper to prevent combinatorial explosion. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
owen-mc
reviewed
Apr 24, 2026
| entryNode(entry) and | ||
| iDomEffect(entry, result) and | ||
| iDomEffect*(result, node) | ||
| private ControlFlow::Node mostRecentSideEffect(ControlFlow::Node entry, ControlFlow::Node node) { |
Contributor
There was a problem hiding this comment.
Please update the QLDoc to explain what the new parameter does.
| * Holds if `access` is an access to a variable `target` for which SSA information is incomplete. | ||
| */ | ||
| private predicate analyzableOtherVariable(DataFlow::Node access, ValueEntity target) { | ||
| private predicate analyzableOtherVariable( |
Contributor
There was a problem hiding this comment.
Please update the QLDoc to explain what the new parameter does.
owen-mc
reviewed
Apr 24, 2026
| @@ -181,15 +182,21 @@ private predicate iDomEffect(ControlFlow::Node dominator, ControlFlow::Node node | |||
| * The immediate dominator path to line 015 is 000 - 009 - 012 - 015. | |||
| * Therefore, the most recent side effect for line 015 is line 009. | |||
Contributor
There was a problem hiding this comment.
Suggested change
| * Therefore, the most recent side effect for line 015 is line 009. | |
| * Therefore, the most recent side effect for line 015 is line 009. | |
| * (Note that line 009 is not a side-effect itself. Instead, it is the | |
| * point where the control flow paths from the side-effects at 004 and 007 | |
| * merge. Because its immediate dominator is the entry node 000, it serves | |
| * as the safe root for expressions evaluated after those side-effects.) |
I found the QLDoc for this predicate quite hard to understand. I asked copilot to make it clearer and it suggested this.
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
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.
I'm not sure why multiple entry points can happen in the first place (likely TRAP deduplication), but when they do, we need to prevent them from getting mixed in the computation of
mostRecentSideEffect. In one example,mostRecentSideEffectwould contain 3,095,056,949 rows, whereas after this PR it only contains 3,628,500 rows.