Skip to content

fix: ins ror_val out of range#477

Merged
eigmax merged 5 commits intopre-release-v1.2.5from
fix/ins-offset-32
Mar 17, 2026
Merged

fix: ins ror_val out of range#477
eigmax merged 5 commits intopre-release-v1.2.5from
fix/ins-offset-32

Conversation

@eigmax
Copy link
Copy Markdown
Member

@eigmax eigmax commented Mar 16, 2026

The INS instruction shifted ror_val right by msb - lsb + 1. When
msb=31, lsb=0, the shift amount is 32, exceeding the ShiftRight chip's [0,
31] range limit.

Fix 1: Split INS SRL into two steps (across multiple files)

Problem: The INS instruction shifted ror_val right by msb - lsb + 1. When
msb=31, lsb=0, the shift amount is 32, exceeding the ShiftRight chip's [0,
31] range limit.

Fix: Split the single >> (msb - lsb + 1) into two steps:

  • srl1_val = ror_val >> 1 (always 1, in range)
  • srl_val = srl1_val >> (msb - lsb) (shift amount ∈ [0, 31])

Fix 2: add.rs:43-44 debug_assert correction (just now)

Problem: The debug_assert for byte 3 overflow omitted carry[2]:
// Old: missing carry[2]
let overflow = a[3].wrapping_add(b[3]).wrapping_sub(expected[3]) as u32;
When byte 2 produces a carry (carry[2]=1) but byte 3 doesn't overflow,
overflow evaluates to 255 instead of 0, triggering the assertion.

Fix: Use correct u32 arithmetic including carry[2]:
// New: correct
let overflow = (a[3] as u32) + (b[3] as u32) + (carry[2] as u32) -
(expected[3] as u32);
debug_assert!(overflow == 0 || overflow == 256);

The AIR constraint in eval() (line 73: overflow_3 = a[3] + b[3] - value[3] +
carry[2]) was always correct. Only the debug_assert in populate had the
latent bug, which was exposed by the new intermediate values from Fix 1.

@eigmax eigmax changed the base branch from main to pre-release-v1.2.5 March 16, 2026 06:34
@eigmax eigmax merged commit ad42744 into pre-release-v1.2.5 Mar 17, 2026
@eigmax eigmax deleted the fix/ins-offset-32 branch March 17, 2026 06:37
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.

4 participants