Skip to content

fix(ir_lower): defer the release-before-store for storage that can still widen - #1011

Merged
nahime0 merged 2 commits into
mainfrom
fix/479-deferred-release-before-store
Sep 18, 2026
Merged

nahime0 merged 2 commits into
mainfrom
fix/479-deferred-release-before-store

Conversation

@Guikingone

Copy link
Copy Markdown
Collaborator

Closes #479.

What was wrong

<?php
for ($n = 0; $n < 50; $n++) {
    try { throw new TypeError("x"); } catch (\Throwable $e) {
        $e = new TypeError("y");
    }
}
echo "done";
HEAP DEBUG: allocs=300 frees=153 live_blocks=147 live_bytes=7840

The diagnosis in the issue is inverted, and that is where the fix lives

The issue reads the EIR's load_local slot[0] typed php=mixed as "the release misreads an object pointer as a box". It is the other way round — and the distinction moves the fix to a different layer entirely.

catch (\Throwable $e) binds the slot as Object("Throwable"). The later TypeError store widens it: two object types have no arm in widened_local_storage_type, so the pair falls through to _ => Mixed. The backend then uses that final storage type for the whole frame — every store boxes, every load unboxes.

The release emitted before the first store was lowered while the slot still looked concrete:

v11: Heap(Object) php=Throwable = load_local slot[1]   ; lowered before the widening
release v11
store_local v10 slot[1]

Against boxed storage, an Object-typed load is an unbox with retain: it hands back the inner pointer with a fresh reference. The release cancels exactly that reference — and the box is never freed. Two blocks per iteration: the box, and the object it pinned.

Fix

That stale-type shape is what release_local_slot already exists for. #534 hit it for a slot that looked untracked (an inner for counter widened IntMixed by its checked-add update), and the deferred op fixed it by letting the backend release at the final widened type. A storage type that already needs lifetime tracking goes stale the same way — it was simply excluded by an early return.

So inside a loop the deferred op is now used for every storage type that can still widen:

  • Mixed / Union keep the eager path — they are terminal, nothing widens a box further, so their eager load is already correctly typed.
  • Str keeps it too, deliberately. Its eager release runs through the ownership analysis, which knows a .rodata literal pointer must not be freed; the slot-typed cleanup does not model that. Moving strings onto the deferred path would trade a leak for a free of read-only memory. (StrMixed therefore still has the same latent shape — worth its own issue, not this one.)

It is not catch-specific

The issue says the release path "falls back to Mixed for catch-bound locals specifically". It does not. Any supertype-typed local reassigned to a subtype leaks identically:

<?php
interface Shape { public function area(): int; }
class Sq implements Shape { public function area(): int { return 4; } }
function make(): Shape { return new Sq(); }

for ($n = 0; $n < 50; $n++) {
    $s = make();
    $s = new Sq();
}
HEAP DEBUG: allocs=200 frees=102 live_blocks=98 live_bytes=3136

That shape has its own regression test, because a catch-only fix would have left it leaking.

The fix I did not ship

The tempting one-liner is to give widened_local_storage_type the missing (Object, Object) arm, so two classes share one pointer slot instead of widening to Mixed — matching the Array/Array arm right above it. It fixes both repros.

It also breaks twelve DatePeriod tests. The synthetic DatePeriod bodies assign a DateTime in one branch and a DateTimeImmutable in the other:

if ($this->startIsImmutable) { $d = new DateTimeImmutable(); return $d->setTimestamp($this->startTs); }
$d = new DateTime();

which the checker rejects in userland — cannot reassign $d from DateTimeImmutable to DateTime — and which only works because the slot boxes. (It fails as Call to a member function format() on null, and the load side needs a matching arm in local_load_types_share_storage before it even compiles.) Widening stays; the release learns to wait for it.

After

HEAP DEBUG: allocs=300 frees=300 live_blocks=0 live_bytes=0
HEAP DEBUG: leak summary: clean

Tests

tests/codegen/runtime_gc/object_supertype_rebind.rs: the issue's repro, the interface-typed local, and the issue's three controls ($e = null, unset($e), same-class reassignment).

Both repros were verified to fail with the fix reverted, and the controls to pass either way. The controls use distinct variable names on purpose — sharing one name makes the third loop widen the first loop's slot, which turns the control into a fourth repro. It did, while I was writing it.

cargo test --test codegen_tests for codegen::runtime_gc (301), codegen::oop (615) and codegen::oop::datetime (135) pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr

@github-actions github-actions Bot added area:eir Touches EIR definitions, lowering, validation, or passes. size:s Small pull request. type:fix Corrects broken or incompatible behavior. labels Sep 14, 2026
@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown

Greptile Summary

This PR corrects ownership cleanup for loop-local storage whose representation can widen after an earlier store is lowered.

  • Defers release-before-store until the backend knows the slot's final widened storage type.
  • Preserves eager cleanup for terminal boxed types, strings, reference-bound locals, and stores outside loops.
  • Adds heap-debug regressions for catch-bound and interface-typed object reassignment, plus unaffected control cases.
  • Documents the widening and ownership behavior in the memory model.

Confidence Score: 5/5

The PR appears safe to merge; the previous documentation and misleading-comment findings are resolved, and no new actionable issue remains.

The current implementation defers cleanup only where loop lowering can observe a stale pre-widening storage type, retains eager ownership-aware cleanup where required, and includes focused heap-debug coverage for the reported leak and its controls.

Important Files Changed
Filename Overview
src/ir_lower/context.rs Defers loop-local cleanup when the current storage representation may still widen, while retaining the existing specialized cleanup paths.
tests/codegen/runtime_gc/object_supertype_rebind.rs Adds focused heap-debug regressions and controls for object-supertype slots that widen to boxed Mixed storage.
tests/codegen/runtime_gc.rs Registers the new runtime-GC regression module.
docs/internals/memory-model.md Documents deferred cleanup for both initially untracked and already-tracked storage that later widens.
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Lower store inside loop] --> B{Local is reference-bound?}
    B -->|Yes| C[Use reference-cell ownership path]
    B -->|No| D{Storage can still widen?}
    D -->|Yes| E[Emit deferred release_local_slot]
    D -->|No| F{Current storage needs tracking?}
    F -->|Yes| G[Emit eager typed release]
    F -->|No| H[No release needed]
    E --> I[Backend resolves final slot representation]
    I --> J[Release boxed or concrete value correctly]
Loading

Reviews (4): Last reviewed commit: "docs(internals): record the deferred rel..." | Re-trigger Greptile

Comment thread src/ir_lower/context.rs
Comment thread tests/codegen/runtime_gc/object_supertype_rebind.rs
…ill widen

Closes #479.

Reassigning a caught exception to a new object leaked both objects, about
three blocks per iteration:

    for ($n = 0; $n < 50; $n++) {
        try { throw new TypeError("x"); } catch (\Throwable $e) {
            $e = new TypeError("y");
        }
    }
    // HEAP DEBUG: allocs=300 frees=153 live_blocks=147 live_bytes=7840

The issue reads the EIR's `load_local slot[0]` typed `php=mixed` as the release
misreading an object pointer as a box. It is the other way round, and the
distinction is what makes the fix land somewhere else entirely.

`catch (\Throwable $e)` binds the slot as `Object("Throwable")`. The later
`TypeError` store widens it -- two object types have no arm in
`widened_local_storage_type`, so the pair falls through to `Mixed` -- and the
backend uses the FINAL storage type for the whole frame. The release emitted
before the FIRST store was lowered while the slot still looked concrete, so it
loads at `Object`, which against boxed storage is an unbox: it hands back the
inner pointer with a fresh reference, the release cancels exactly that
reference, and the BOX is never freed. Two blocks per iteration, the box and
the object it pinned.

That stale-type shape is the one `release_local_slot` already exists for. Issue
#534 hit it for a slot that looked UNTRACKED (an inner `for` counter widened
Int->Mixed by its checked-add update), and the deferred op fixed it by letting
the backend release at the final widened type. A storage type that already
needs lifetime tracking goes stale the same way; it was simply excluded by an
early return.

So inside a loop the deferred op is now used for every storage type that can
still widen. `Mixed`/`Union` keep the eager path because they are terminal --
nothing widens a box further, so their eager load is already correctly typed.
`Str` keeps it too, deliberately: its eager release runs through the ownership
analysis, which knows a `.rodata` literal pointer must not be freed, and the
slot-typed cleanup does not model that. Moving strings onto the deferred path
would trade a leak for a free of read-only memory.

The issue reports this as catch-specific. It is not. Any supertype-typed local
reassigned to a subtype leaks identically, which is the second regression test
here:

    interface Shape { ... }  class Sq implements Shape { ... }
    function make(): Shape { return new Sq(); }
    for (...) { $s = make(); $s = new Sq(); }
    // HEAP DEBUG: allocs=200 frees=102 live_blocks=98 live_bytes=3136

A first attempt instead gave `widened_local_storage_type` the missing
`(Object, Object)` arm, so two classes would share one pointer slot rather than
widening to `Mixed`. It fixes both repros and is tempting, but it breaks twelve
`DatePeriod` tests: the synthetic `DatePeriod` bodies assign a `DateTime` in one
branch and a `DateTimeImmutable` in the other -- which the checker REJECTS in
userland ("cannot reassign $d from DateTimeImmutable to DateTime") and which
only works because the slot boxes. Widening stays; the release learns to wait
for it.

Tests: the issue's repro, the interface-typed local that shows it is not
catch-specific, and the issue's three controls (`$e = null`, `unset($e)`,
same-class reassignment). Both repros were verified to FAIL with the fix
reverted and the controls to pass either way -- they use distinct variable
names on purpose, since sharing one name makes the third loop widen the first
loop's slot and turns the control into a fourth repro.

`cargo test --test codegen_tests` for `codegen::runtime_gc` (301),
`codegen::oop` (615) and `codegen::oop::datetime` (135) pass.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
…st docs

Review follow-up on #479, on two counts, both correct.

The regression file's Rustdoc described the fix I did NOT ship. It said the
change stops a subtype store from widening the slot, and that the release reads
a raw object pointer as a box. Both are inverted: the widening is deliberate and
stays, and the release is not a no-op on a pointer but an unbox WITH RETAIN
against boxed storage -- it hands back the inner pointer holding a fresh
reference, the release cancels exactly that reference, and the box is never
freed.

That text came from a first attempt that gave `widened_local_storage_type` the
missing `(Object, Object)` arm. It fixes both repros and breaks twelve
`DatePeriod` tests, whose synthetic bodies assign a `DateTime` in one branch and
a `DateTimeImmutable` in the other and rely on the box. I changed the approach
and not the comments, which is exactly the trap the reviewer names: a future
ownership change reading those tests would have aimed at the wrong behaviour.
The abandoned approach is now recorded alongside the shipped one, so the next
person does not rediscover it.

`docs/internals/memory-model.md` gains the second shape on its variable-
reassignment bullet, which already carried #534's untracked-slot case. An
already-tracked storage type goes stale the same way and is the more dangerous
of the two, for the retain reason above. The two exclusions are written down
with their reasons: `Mixed`/`Union` is terminal, and `Str` keeps the eager path
deliberately because its release runs through the ownership analysis, which
knows a `.rodata` literal pointer must not be freed.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Guikingone added a commit that referenced this pull request Sep 16, 2026
`curl_monitor_excludes_write_callback_cpu_from_network_wait` failed on four
unrelated pull requests in one afternoon -- #1011 (an `ir_lower` release-ordering
change), #1012 (`array_fill` argument boxing), #1017 and #1019 (test-only
additions). None of them touches curl, the monitor, or timing.

The measurement is still doing its job; the slack is what does not survive a
shared runner. The control stayed at ~0.42 ms every time while the instrumented
run came back at:

    5 560 376 ns
    7 802 876 ns
    9 111 124 ns

against `WAIT_SLACK_NS = 5_000_000`. That is loopback scheduling jitter, not
billed callback CPU: the burn is ~130 ms, so billed CPU would show as ~130 ms,
which is exactly what the defect reported (133.9 ms against a 1.2 ms control).

20 ms covers the observed values with better than 2x to spare and still leaves
~6.6x below the 132.7 ms gap the defect produced. The test keeps failing
outright on the regression it guards -- two orders of magnitude is not a
threshold anyone has to tune -- while no longer failing on the machine it runs
on.

Nothing else changes: `BURN_ROUNDS`, the `total > 20_000_000` floor that rejects
a fixture too short to measure, and the `after_wait > 0` control that rejects a
build recording no wait at all are all untouched.

Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
@Guikingone
Guikingone force-pushed the fix/479-deferred-release-before-store branch from 4fa6f8c to c74ea0b Compare September 16, 2026 21:13

@nahime0 nahime0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@nahime0
nahime0 merged commit c0cb119 into main Sep 18, 2026
149 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:eir Touches EIR definitions, lowering, validation, or passes. size:s Small pull request. type:fix Corrects broken or incompatible behavior.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reassigning the catch variable to a new object leaks: previous-value release mistyped as Mixed

2 participants