Skip to content

fix: preserve shared if branch targets - #2936

Open
FuPeiJiang wants to merge 1 commit into
skylot:masterfrom
FuPeiJiang:fix/if-region-shared-targets
Open

fix: preserve shared if branch targets#2936
FuPeiJiang wants to merge 1 commit into
skylot:masterfrom
FuPeiJiang:fix/if-region-shared-targets

Conversation

@FuPeiJiang

Copy link
Copy Markdown

Description

Fix IfRegionMaker moving shared branch targets outside an if when those blocks also have incoming edges from an enclosing control-flow structure.

The branch check now:

  • only promotes a shared branch to the if continuation when sibling fall-through paths prove it is the continuation;
  • rejects findOutBlock() candidates beyond an active enclosing region exit.

Adds regression coverage for shared branch targets both with and without a switch.

A related but different region-making issue is tracked in #2909; its secondary-break CFG is not fixed by this change.

Validation

  • ./gradlew clean build dist --no-daemon

Keep branch blocks with external predecessors inside the current if unless sibling fall-through proves they are the continuation. Reject out-block candidates beyond an active enclosing region exit.

Add switch and non-switch regression coverage for shared branch targets.
@FuPeiJiang

Copy link
Copy Markdown
Author

More detail on the failure mode and why the fix is intentionally not switch-specific.

Failure mode

The original reproducer has a block that is a legitimate branch target of an inner if, but is also reached directly from an enclosing control-flow construct. A reduced shape is:

             outer control flow
              /            \
             /              \
        COMPLETE          INNER_IF
                           /     \
                    COMPLETE    STOP -> FAIL

For the inner if, COMPLETE and STOP are the two real branches. However, COMPLETE has an additional predecessor from outside the current if.

Previously, allPathsFromIf() effectively treated this as exclusive ownership:

branch_of(B, IF)
    =>
every non-loop predecessor of B belongs to IF.mergedBlocks

That is too strong for shared CFG targets. In the example, the external predecessor makes allPathsFromIf(COMPLETE) return false even though COMPLETE is still a legitimate branch of the inner if.

restructureIf() then classifies that branch as "bad", inverts the condition, and promotes the original branch block to outBlock. Structurally this changes:

if condition:
    COMPLETE
else:
    STOP
    FAIL

into the equivalent of:

if !condition:
    STOP
    FAIL
COMPLETE

which makes COMPLETE unconditional and changes program semantics.

Why this does not special-case switches

A first narrow fix could ignore an external predecessor when it is a SwitchInsn that directly targets the branch. That fixes one reproducer, but it encodes the source of the extra edge instead of the property needed for a safe transformation.

The same invalid promotion can occur when the external predecessor comes from another if or another shared region. The added non-switch regression covers exactly that case.

The relevant property is instead whether the sibling branch proves that the candidate is actually a continuation. The new logic only promotes a shared branch when all non-terminating, non-back-edge sibling paths stay in the current dominated scope until they reach that branch. A return/throw-style terminal path is allowed to end without reaching it.

In other words, an external incoming edge no longer means "this must be the continuation". Promotion requires positive CFG evidence that it is safe to execute the candidate after the reconstructed if.

Enclosing-region exit case

There is a second related failure mode when findOutBlock() returns a block at or beyond an exit already owned by an enclosing region. RegionStack then marks that candidate as the nested if exit, and a legitimate nested branch targeting the same block can be suppressed as an empty region.

The patch rejects such outBlock candidates when an active enclosing exit can reach them. This keeps nested region construction within its actual scope.

Semantic invariant

The transformation should preserve this invariant:

Promoting branchBlock to outBlock must not introduce a path to branchBlock
that did not reach branchBlock in the original CFG.

Equivalently, code emitted after a reconstructed if must be a continuation for every non-terminating path that falls through that if, not merely a block with a convenient predecessor/frontier shape.

Regression coverage

Two minimized smali regressions are included:

  • TestSwitchSharedCaseTargets: shared target between a switch case and an inner if branch.
  • TestSharedIfBranchTarget: the same class of bug without any switch, using another conditional/shared target. This one uses runDecompiledAutoCheck so the generated Java is compiled and executed, checking semantics rather than only source shape.

Validation on the branch:

./gradlew clean build dist --no-daemon
BUILD SUCCESSFUL

Relation to #2909

#2909 is related because it also produces an incorrect IfRegionMaker boundary and can make conditional code unconditional, but it fails at a different decision point. There, findOutBlock() chooses an intermediate merge block even though another continuing branch bypasses that block and rejoins later.

This PR fixes shared branch ownership/continuation and enclosing-scope validation; it does not fix the partial-merge selection in #2909, so I have intentionally not marked #2909 as fixed by this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant