Skip to content

codegen: js_eq's left operand is a stale register across its sibling's allocating lowering (residual #7217 allocation-point witness) #7248

Closed

Description

@proggeramlug

Summary

test_gap_gc_assign_string_source_rooting is improved but not green by #7217's fix: bad char 3 count 3 → bad char 1 count 1 on the allocation-point arm, both 10/10 deterministic on the same build lineage. Two of the three bad iterations were the lazy globalThis bootstrap #7217 closed. The residual one is a codegen-side stale operand in the test's own assertion, not in Object.assign at all.

So the ten assign_string_source entries in test-parity/gc_repsel_triage.txt are retargeted here, not deleted — and their stated cause ("allocation-point relocation inside js_object_assign_one / object_assign_string_source") is now known to be wrong.

Reproduction

export PERRY_RUNTIME_DIR=<build>/perry-dev PERRY_NO_AUTO_OPTIMIZE=1
perry test-files/test_gap_gc_assign_string_source_rooting.ts -o /tmp/asrc   # no compile-time env
PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off /tmp/asrc
# bad char 1 count 1     (oracle: bad char 0 count 0)
build result, 10/10
merge-base 8b024958f bad char 3 count 3
with #7217's no-move bootstrap window bad char 1 count 1

The site, named by #7196's quarantine

[gc-fromspace-protect] FAULT: signal 10 at 0x5484173ffac
  block=0x54841640000 +1048492 retired_bytes=1048512 retired_by_minor=#2
  last-known object: user_ptr=0x5484173ffa8 obj_type=3 size=32
2  js_jsvalue_equals + 332
3  js_eq + 20
4  main + 1028

obj_type=3 is a string; retired_by_minor=#2 (not #0), which is why it survived the bootstrap fix.

Why

The failing line is the test's own comparison:

if (got !== ALPHA[i % 26]) { badChar++; break; }

got (a copied property read) is lowered first; ALPHA[i % 26] is a string index read, which allocates a fresh one-character string. So the left operand sits in a register across its sibling's allocating lowering and js_eq hands js_jsvalue_equals a from-space StringHeader*, which it then dereferences to compare bytes.

The emitted IR says it without inference (--trace llvm, no compile-time GC env):

  %r136 = load double, ptr %r97                    ; `got` -- loaded ABOVE
  ...
  %r149 = call double @js_string_index_get_boxed(double %r137, double %r148)  ; ALLOCATES
  %r150 = bitcast double %r136 to i64              ; the STALE register
  %r151 = bitcast double %r149 to i64
  %r152 = call i64 @js_eq(i64 %r150, i64 %r151)

%r97 may well be a shadow-bound alloca that the collector rewrites -- but %r136 is a register taken before the collection point and it is never re-read. That is the third property a root must buy (#7207's words: liveness, a rewritten location, and the value the call actually observed), and only the first two are present.

That is exactly the family #7206 (a call receiver and a computed-read base across their sibling operands) and #7214 (the callee, this and every argument of js_closure_callN) closed for other call shapes. js_eq / js_strict_eq / the relational operators take two lowered operands and were apparently not swept in either pass: guard_store_operand_across / RootedOperands::reread exist for precisely this and are not applied here.

Why it matters beyond the witness

a !== b where either side allocates is ordinary JS. The reason this is only visible on the allocation-point route is that on the safepoint route the sibling's allocation is not a collection point — which is the #7217 route argument, restated for a codegen site rather than a runtime one.

Suggested shape

Sweep the binary-operator lowerings (js_eq, js_strict_eq, js_lt/le/gt/ge, js_add where it can be a string concat) with the same guard_store_operand_across treatment #7206 gave the call receiver: root the first-lowered operand whenever the second's lowering collects, and re-read it below. A codegen regression test in the #7206/#7214 style (assert the emitted IR re-reads the left operand below the sibling's allocating call) is the gate.

Refs #7217, #7216, #7206, #7214, #7196.

Activity

  1. proggeramlug commented on Aug 2, 2026

    @proggeramlug
    ContributorAuthor

    Still open on main = d4ac65f21 — but the numbers in this issue are no longer correct, and the difference matters.

    Re-verified as part of the sweep in #7262. Same build recipe as this issue specifies (--profile perry-dev, PERRY_RUNTIME_DIR pinned, PERRY_NO_AUTO_OPTIMIZE=1, compiled with no compile-time GC env), same arm:

    PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off ./asrc
    
    this issue states measured at d4ac65f21
    result bad char 1 count 1, 10/10 deterministic bad char 2 count 2 6/10, bad char 3 count 3 4/10

    Movement on the same binary and arm: 3 copying minors, 57 318 objects copied. Oracle (node 26.5.1) is bad char 0 count 0.

    Two corrections

    1. It is not deterministic. This issue and test-parity/gc_repsel_triage.txt both describe a fixed 1, 10/10. It is a distribution — 2 or 3 per run. Anyone bisecting against "did it go from 1 to 0" will read noise as signal; the acceptance criterion has to be bad char 0 count 0 across N runs, not a count comparison.
    2. The residual is larger than one iteration. The issue's framing — "two of the three bad iterations were the bootstrap fix(gc): run the globalThis bootstrap in a no-move window (#7217) #7249 closed, the residual one is the js_eq operand" — implied a single remaining site. At 2–3 bad iterations post-fix(gc): run the globalThis bootstrap in a no-move window (#7217) #7249, either the stale-js_eq-operand site fires more than once, or there is a second site. The js_eq diagnosis itself is well-evidenced (the --trace llvm extract showing %r136 loaded above the allocating js_string_index_get_boxed and never re-read, plus the js_jsvalue_equals + 332 quarantine frame) and is not in question — but it should not be assumed to be the whole residual.

    The triage entries in test-parity/gc_repsel_triage.txt quoting bad char 1 count 1 for all ten arms are stale on the same grounds.

    Not fixing here — read-only sweep.

  2. proggeramlug commented on Aug 3, 2026

    @proggeramlug
    ContributorAuthor

    Heads-up before anyone invests here — this issue is in a class that a planned architectural change would make unrepresentable, and I don't want someone building new machinery that gets deleted.

    See docs/src/internals/rfc-rooting-by-construction.md ("Where this sits in the one GC correctness plan", landed in #7292). Short version: this is layer 2 — a GC-managed pointer live in emitted machine code across a point where the collector can run. Statepoints/stack maps (#7108, #7174, enabled by #7241) would have LLVM track that liveness precisely, so the whole shape stops being expressible.

    This is not "don't fix it". The architectural change is weeks away, the bug is real now, and correctness comes first — please do fix it.

    It is "fix the instance, don't build new mechanism." Specifically:

    • A targeted root/RuntimeHandleScope/GcSuppressScope at the site: good, lands now, survives.
    • A new predicate, a new shadow-slot allocation scheme, or a new hand-maintained table deciding what to root: please raise it first. codegen: ClassExprFresh leaves the class object unrooted across its own js_object_set_field_by_name stores #7211 is the cautionary case — a four-clause predicate written by someone actively thinking about rooting, still wrong, because it asked about the user's expressions rather than the lowering's own emitted calls. Statepoints delete that entire category of judgement call.

    Also worth knowing when you verify: PERRY_GC_MOVING_LOOP_POLLS=1 set at run time against a default-compiled binary is clean 10/10 and proves nothing — the poll codegen isn't in the binary. Compile-time and run-time env must both be set (#7280). And a green run that performed zero copying minors proves nothing either (#7255) — assert copied_objects > 0.

  3. proggeramlug commented on Aug 10, 2026

    @proggeramlug
    ContributorAuthor

    Does not reproduce on main @ db44b31b7, and the IR shows why: the stale register is gone — the left operand is re-read below the sibling's allocating call.

    The witness

    Exactly the reproduction in the issue (perry-dev, PERRY_NO_AUTO_OPTIMIZE=1, no compile-time GC env, then PERRY_GC_HEAP_LIMIT=8 PERRY_GC_INCREMENTAL=0 PERRY_CONSERVATIVE_STACK_SCAN=off):

    build result
    this issue, at 8b024958f bad char 3 count 3, 10/10
    this issue, with #7217 bad char 1 count 1, 10/10
    main today bad char 0 count 0, 10/10 — the oracle

    Checked that the pass is not vacuous, because a GC witness that stops collecting is the failure mode this repo pays for most: the same run under PERRY_GC_DIAG=1 reports 3 collections, all 3 copying minors, freeing 1.3 / 3.5 / 5.0 MB. It collects, relocates, and still gets the right answer.

    The IR, which is the actual answer

    The issue's diagnosis was a register taken above an allocating call and never re-read:

      %r136 = load double, ptr %r97                    ; `got` — loaded ABOVE
      %r149 = call double @js_string_index_get_boxed(…) ; ALLOCATES
      %r150 = bitcast double %r136 to i64              ; the STALE register
      %r152 = call i64 @js_eq(i64 %r150, i64 %r151)

    Emitted today for the same source, same flags:

      %r144 = call double @js_string_index_get_boxed(double %r132, double %r143)  ; ALLOCATES
      %r193.rs4p = load ptr addrspace(1), ptr %r86     ; RE-READ, below the call
      %r193.rs4i = ptrtoint ptr addrspace(1) %r193.rs4p to i64
      %r193 = bitcast i64 %r193.rs4i to double
      %r194 = bitcast double %r193 to i64
      %r147 = call i64 @js_eq(i64 %r194, i64 %r146)

    The .rs4p / .rs4i suffixes are the RS4GC retype pass: the operand is a relocatable ptr addrspace(1) reloaded from its slot after the allocation point, so js_eq observes the post-collection value. That is the third property #7207 names — liveness, a rewritten location, and the value the call actually observed — and all three are now present at this site.

    What I did and did not do

    I implemented the suggested fix first — rooting::with_operands_rooted over [left, right] in both the js_eq/js_strict_eq and the js_rel_* fallback arms of expr/compare.rs — and it passes 10/10 too. Then I A/B'd it against an unmodified build and the baseline also passes 10/10, so the change fixes nothing that is currently broken. It is reverted rather than shipped: adding operand rooting where RS4GC already re-reads is pure IR cost, and this repo's own rule is that a rooting change has to be justified by a window that exists.

    Closing as fixed-elsewhere. If it returns, the diagnosis in this issue is still the right one and the fix shape is still with_operands_rooted on those two arms — the thing to check first is whether the .rs4p re-read below js_string_index_get_boxed is still being emitted, since that is what closed it.

    The ten assign_string_source entries in test-parity/gc_repsel_triage.txt that were retargeted here can go.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions