Skip to content

improve asm sucessor to deal with arbritary jumps#7568

Draft
xunilrj wants to merge 2 commits intomasterfrom
xunilrj/improve-asm-successor
Draft

improve asm sucessor to deal with arbritary jumps#7568
xunilrj wants to merge 2 commits intomasterfrom
xunilrj/improve-asm-successor

Conversation

@xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Mar 6, 2026

Description

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@cursor
Copy link

cursor bot commented Mar 6, 2026

PR Summary

Medium Risk
Touches CFG successor computation used by liveness analysis and reachability-based optimizations, which can impact register allocation and dead-code elimination correctness. Risk is mitigated by conservatively bailing out of optimizations when successor sets are Unknown (e.g., JumpToAddr, potential ECAL).

Overview
Introduces a new InstructionSuccessor enum and changes Op::successors() (and virtual/control-flow successor helpers) to return explicit successor semantics instead of a raw Vec<usize>, distinguishing Unknown, ReturnFromCall, and Many successor sets.

Updates liveness analysis and CFG reachability simplification to consume this new API; reachability now conservatively disables CFG simplification when successors are Unknown (and similarly bails early on JumpToAddr). Snapshot output for main_args_various_types is updated to reflect resulting register allocation changes.

Written by Cursor Bugbot for commit 1327cf7. This will update automatically on new commits. Configure here.

@xunilrj xunilrj temporarily deployed to fuel-sway-bot March 6, 2026 11:33 — with GitHub Actions Inactive
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Off-by-one in assertion allows invalid index
    • Changed the successors assertion to index < ops.len() so invalid end/empty-slice indices are rejected before successor computation.

Create PR

Or push these changes by commenting:

@cursor push ce836f34a4
Preview (ce836f34a4)
diff --git a/sway-core/src/asm_lang/virtual_ops.rs b/sway-core/src/asm_lang/virtual_ops.rs
--- a/sway-core/src/asm_lang/virtual_ops.rs
+++ b/sway-core/src/asm_lang/virtual_ops.rs
@@ -963,7 +963,7 @@
     /// ECAL can do pretty much anything, but it executes the next instruction, if "$pc"
     /// is not manipulated.
     pub(crate) fn successors(&self, index: usize, ops: &[Op]) -> InstructionSuccessor {
-        assert!(index <= ops.len());
+        assert!(index < ops.len());
         match self {
             VirtualOp::RET(..) | VirtualOp::RETD(..) | VirtualOp::RVRT(..) => {
                 InstructionSuccessor::Many(vec![])
This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

/// ECAL can do pretty much anything, but it executes the next instruction, if "$pc"
/// is not manipulated.
pub(crate) fn successors(&self, index: usize, ops: &[Op]) -> InstructionSuccessor {
assert!(index <= ops.len());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off-by-one in assertion allows invalid index

Low Severity

The assertion assert!(index <= ops.len()) uses <= instead of <. Since index represents a valid instruction index into ops, the invariant is index < ops.len(). Using <= allows index == ops.len() to pass the assert silently. Worse, if ops is empty and index == 0, the assert passes but ops.len() - 1 on line 974 causes a usize underflow panic. With <, the assert would correctly catch both cases.

Fix in Cursor Fix in Web

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 6, 2026

Merging this PR will not alter performance

✅ 25 untouched benchmarks


Comparing xunilrj/improve-asm-successor (1327cf7) with master (cf207d2)

Open in CodSpeed

@xunilrj xunilrj force-pushed the xunilrj/improve-asm-successor branch from fa5d11d to 1327cf7 Compare March 18, 2026 17:41
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