Skip to content

i32 fast path: a >>> 0 uint32 copied into a local with a signed literal seed takes a SIGNED i32 slot and reads back negative #6359

Description

@proggeramlug

Severity: P1 (silent data corruption)
Found while fixing: #6339

What

A >>> 0 result (a uint32, range 0 .. 2^32-1) assigned into a local whose other write is a plain signed integer literal gets a signed i32 shadow slot. Reads sitofp the bit pattern, so any value ≥ 2^31 comes back negative.

const SEED = 0x9e3779b9 >>> 0;

let seedCopy = 0;
seedCopy = SEED >>> 0;
console.log(seedCopy);   // node: 2654435769   perry: -1640531527

let d = 0;
d = 4000000000 >>> 0;
console.log(d);          // node: 4000000000   perry: -294967296

Reproduced against origin/main, against the #6319 branch (#6340) and against the #6339 branch — all three print the same wrong values, so it is independent of that family's fixes.

What still works (so this is narrow)

const SEED = 0x9e3779b9 >>> 0;
console.log(SEED);                       // 2654435769 ✓ (const, no i32 slot at all)

let s = SEED >>> 0;                      // EVERY write is `>>> 0`
s = (s ^ ((s << 13) >>> 0)) >>> 0;
s = (s ^ (s >>> 17)) >>> 0;
console.log(s);                          // ✓ correct — unsigned_i32_locals path

image_convolution's xorshift s is exactly the second shape, which is why the compiler-output-regression gate never caught this.

Why

Two sets disagree about signedness.

  • collect_unsigned_i32_locals (crates/perry-codegen/src/collectors/i32_locals.rs) requires every write to be expr >>> 0. Those locals get the unsigned slot and are read back with uitofp. seedCopy's Let init is 0 — a plain Expr::Integer, not a >>> 0 — so it is disqualified.
  • is_strictly_i32_bounded_expr accepts expr >>> 0 as strictly-i32-bounded:
Expr::Binary { op, right, .. }
    if matches!(op, BinaryOp::BitOr | BinaryOp::UShr)
        && matches!(right.as_ref(), Expr::Integer(0)) => true,

But | 0 is ToInt32 (signed i32 — genuinely bounded) while >>> 0 is ToUint32 (unsigned). Both writes to seedCopy therefore pass, seedCopy lands in strictly_i32_bounded_locals, needs_i32_slot gives it the signed i32 shadow at its Let site, and every read sitofps 0x9E3779B9 to -1640531527.

So the local falls between the two stools: too mixed for the unsigned slot, wrongly admitted to the signed one.

Also suspect

The general bitwise arm just below returns true for any UShr:

Expr::Binary { op, .. } => matches!(op, BitAnd | BitOr | BitXor | Shl | Shr | UShr),

x >>> k for a constant k >= 1 really does fit in signed i32 (max 2^31 - 1). But x >>> k with a variable k can be a shift by 0 at runtime, which is again a uint32 — same corruption, no literal 0 in the HIR to key on. Worth covering in the same fix.

Fix sketch

Treat "fits in signed i32" and "is a 32-bit bit pattern" as different properties:

  1. Drop BinaryOp::UShr from the right == Integer(0) arm of is_strictly_i32_bounded_expr>>> 0 does not prove a signed i32.
  2. In the general bitwise arm, admit UShr only when the shift amount is a literal >= 1.
  3. Locals that lose the signed slot either qualify for unsigned_i32_locals already or fall back to the f64 slot — correct, marginally slower.

Alternatively, widen collect_unsigned_i32_locals so a local whose writes are a mix of in-range signed literals and >>> 0 casts can still take the unsigned slot. That keeps the fast path but needs the read side to pick uitofp consistently.

Either way it wants the compiler-output-regression gate run: image_convolution's xorshift s and FNV h both live in this machinery.

Confidence: high (probe + source; A/B'd against three separate builds).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugConfirmed defect or regression

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions