fix(codegen): provenance-based transitive disqualification for integer locals (#4785 bug class)#4881
Merged
Merged
Conversation
…r locals (#4785 bug class) The integer-locals analysis now records WHY each candidate is believed integer (its init and every LocalSet rhs, with the local ids each verdict relied on) and prunes transitively via a reverse-dependency worklist when any source is disqualified — no admission path (seed, propagation, clamp_fn_ids, flat_const_ids) can escape. Holes closed (clamp + written-init are live miscompiles on main): - clamp3-shaped calls admitted unconditionally even though clamp3 returns an ARGUMENT verbatim; now argument-dependent with recorded deps - candidates with LocalSet writes were never re-validated through their init (let b = a; use(b); b = 1 kept a stale i32 slot) - Expr::Update was unconditionally int-producing - detect_clamp_u8 never verified its 'return v | 0' statement - lower_call clamp3 intrinsification fptosi'd unproven args at every call site (clamp3(2.5, 0, 5) returned 2); now gated on can_lower_expr_as_i32 - i64 whole-function specialization replaced clamp-shaped bodies with an unconditional fptosi wrapper; clamp shapes are now excluded
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.
Summary
Hardens the integer-locals analysis (
crates/perry-codegen/src/collectors/integer_locals.rs) so disqualification is provenance-based and transitive, and closes two codegen-side holes of the same bug class found while testing end-to-end. The bug class (regression #4785): a local wrongly treated as integer gets an i32 shadow slot / i32 lowering, the f64 value is actually a NaN-boxed pointer (or a fractional double), thefptosiread producesi32::MIN(or a truncated int), and user code crashes with(number).method is not a functionor silently computes wrong values.Two of the closed holes are live miscompiles on current main, demonstrated below.
1. Provenance model in
collect_integer_localsAdmission is unchanged (optimistic syntactic seeds + forward propagation to a fixed point). Disqualification is rebuilt:
Letinit and everyLocalSetrhs targeting it. The only init exemption is the optimisticlet x = undefined; …writes…destructuring-scaffolding seed, whose real values are its writes (a write-freeundefinedinit still fails, as before).int32_producing_deps) records provenance — the exact set of local ids whose candidate-ness each verdict relied on (LocalGet,Updatetargets, clamp-call arguments).The invariant is stated in the module-level comment: no admission path (seed, propagation, clamp_fn_ids, flat_const_ids) escapes transitive disqualification.
This subsumes the previous #4785 fix (
collect_non_int_init_only_let_ids, now removed) and the old per-iterationcollect_non_int_localset_ids_in_stmtsrescan loop (also removed).2. Bypass holes closed
(a) clamp_fn_ids admission — analysis layer.
is_int32_producing_expraccepted any call to aclamp_fn_idsfunction unconditionally, butclamp3-shaped functions return one of their arguments verbatim, soconst xx = clamp3(src, 0, 100)withsrcholding an object keptxxinteger forever (the old init-only re-validation re-accepted the clamp call every iteration). The fact graph now passes the clamp3 set separately (arg_dependent_clamp_fn_ids, plumbed throughcollect_native_region_fact_graphand all six codegen entry points); for those calls every argument must be int-producing and the argument deps are recorded.clampU8-shaped /returns_integerfunctions coerce internally and stay argument-independent, soclampU8(doubleAccumulator)keeps its slot.(b) Init bypass on written locals — analysis layer. Candidates with a
LocalSetwrite were never re-validated through their init: inlet b = a; use(b); b = 1,bstayed integer afterawas disqualified, so the read between init and reassignment saw a truncated pointer. Inits are now obligations for every candidate. Live miscompile on main (see demo).(c)
Expr::Updatebypass — analysis layer.const y = x++was unconditionally int-producing even whenxnever was (or stopped being) an integer. The disqualification judgment now requires the updated local to be a candidate and records the dep (mirrorsis_strictly_i32_bounded_expr).(d) clamp3 call-site intrinsification — codegen layer (
lower_call/func_ref.rs). EveryFuncRefcall to a clamp3 function was lowered tofptosi(args)+llvm.smax/smin+sitofp, violatinglower_expr_as_i32's documented contract ("must be called only aftercan_lower_expr_as_i32returned true"). This truncated NaN-boxed pointers toi32::MINand plain fractional doubles —clamp3(2.5, 0, 5)returned2on main (node returns2.5). The intrinsic is now gated on all three arguments passingcan_lower_expr_as_i32(which reads the hardenedinteger_locals); other calls fall through to the ordinary direct call. TheclampU8intrinsic stays unconditional: with the tightened detector (e) itsfptosi+smax(0)/smin(255)agrees with the verifiedreturn v | 0body for every f64 input.(e)
detect_clamp_u8never checked its third statement. A body ending in barereturn v;passes a fractional in-rangevthrough unchanged, yet callers' results were treated as int-producing (and intrinsified). The matcher now requires the documented int-coercing return (v | 0/ bitwise / integer literal).(f) i64 whole-function specialization of clamp-shaped functions — codegen layer (
codegen/mod.rs).is_integer_specializablematched clamp3-shaped bodies and replaced the compiled function with an i64 variant behind an f64 wrapper thatfptosis all arguments unconditionally. After (d), int-argument call sites use the smax/smin intrinsic and never call the symbol — so the specialized body only ever served exactly the calls it miscompiles. Clamp-shaped functions are now excluded from i64 specialization.Pre-existing, out-of-scope issues found and not changed (flagged for follow-up):
i64s_expreven acceptsExpr::Number(0.5)literals). Same bug class, orthogonal trigger.flat_const_idsare module-init local ids; HIR local ids are scope-local, so a function-local id can in principle collide with a flat-const id. That exposure is shared with codegen'sflat_const_arraysfast-path lowering (which keys on the same ids), so an analysis-only fix would help nothing; documented in the module comment.3. clamp_fn_ids / flat_const_ids scoping audit
clamp_fn_idscontains function ids (clamp3 ∪ clamp_u8 ∪ returns_int), which are module-global — no per-function contamination. The unsoundness was semantic (argument-dependence), fixed in (a)/(d)/(e)/(f).flat_const_idsfacts (never-mutated moduleconstint matrices) are immutable within a per-function analysis, so flat-const admissions correctly carry no local deps — they cannot be invalidated by anything this analysis disqualifies. Collision caveat documented (above).Tests
Unit (
collectors/hir_facts.rs, next to the existing #4785 tests, which still pass unchanged):clamp_admitted_local_is_pruned_when_arg_source_is_disqualified— clamp-admitted chain pruned when an argument source is disqualified; positive int-arg case keeps the optimization; argument-independent (coercing) clamp keeps admitting double args.written_local_is_still_revalidated_through_its_initupdate_admitted_local_follows_its_targetE2E (
crates/perry/tests/integer_locals_provenance.rs, compile withCARGO_BIN_EXE_perry+ run + assert stdout):const [k, v] = entry; const cb = v; cb.setName(...))undefinedseedclampIdx-fed index kernel still compute correct values. The kernel's clamp still lowers to thesmax/sminintrinsic (verified via--trace llvm: int-arg call sites keepfptosi-free i32 chains).Failure-mode demonstration (binary built from this branch's merge-base, i.e. current
main, same sources as the e2e tests):clamp3(2.5, 0, 5)also prints2on main vs node's2.5; this branch matches node byte-for-byte.At the analysis level, the three new unit tests fail without the provenance logic (they encode exactly the holes the old code re-accepted each iteration).
Validation (rebased on current main)
cargo test --release -p perry— 481 unit + 5/5 new e2e + all integration tests (incl. Stripe SDK: stripe.products.create throws 'replace is not a function' (request dispatch, flat body) after #4831 #4841/String.prototype.match(/x/g) segfaults (SIGSEGV) on no-match (global flag) #4858/Codegen:new MessageChannel()global constructor unlinked/non-constructible — routes to stdlib symbol, breaks React scheduler init #4873 regressions, native_link_cache) pass.cargo test --release -p perry-codegen -p perry-hir -p perry-transform -p perry-runtime— pass (95 collector tests incl. all pre-existing fix(codegen): destructured-value copies no longer truncate to i32 (#4766 regression) #4785 regression tests;native_proof_buffer_views21/21).node --experimental-strip-types(loops/updates/number/array subset:test_gap_2308_unrolled_loop_var,test_gap_2530_nonnull_update,test_gap_number_math,test_gap_number_string_2864_2948_2855,test_gap_2792_2814_2794_2796_array_validation) — byte-identical.perry-runtimelib tests: one pre-existing failure (date::tests::test_full_year_setters_revive_invalid_date_only, fails identically at origin/main on this host — environment/TZ-dependent, untouched by this diff); two other tests only fail under parallel execution and pass withRUST_TEST_THREADS=1(the configuration CI uses for this crate).--workspacesuite does not currently build on a macOS host independently of this change: (1)perry-ui-windows-winuiis missing from the documented exclude list and pullswebview2-com-sys; (2) with it excluded, theperry-ext-*lib tests fail to link (Undefined symbols: _js_fetch_with_options) under workspace feature-unification. Both reproduce identically at this branch's merge-base (verified by building the parent commit in the same tree), so the Linuxcargo-testCI gate is the authoritative full-suite check here.Per contributor guidelines, no version bump / CHANGELOG / CLAUDE.md changes — maintainer adds those at merge.