Skip to content

fix(codegen): queue by-ref variadic cleanup before RefWrap commit - #155

Open
yuan-dian wants to merge 3 commits into
swoole:masterfrom
yuan-dian:fix/byref-variadic-local-arg-escape
Open

yuan-dian wants to merge 3 commits into
swoole:masterfrom
yuan-dian:fix/byref-variadic-local-arg-escape

Conversation

@yuan-dian

Copy link
Copy Markdown
Contributor

Body

Fixes a false-positive runtime error when calling a by-ref variadic function/method with plain local-variable arguments. Found while testing the follow-up to #153; master-only repro.

Symptom

function bump(int &...$vals): void {
    foreach ($vals as &$v) {
        $v += 10;
    }
}

function main(): void {
    $a = 1;
    bump($a);
    var_dump($a);
}

Compiles on master, then fails at runtime:

Fatal error: Uncaught Error: A temporary typed reference cannot escape a dynamic call

Write-back should simply succeed (int(11)). The error is a false positive: the caller's slot is a stable local, not a temporary.

Root cause

By-ref variadic arguments are aggregated into a temporary php::Array whose slots hold references back to the caller. The aggregation array therefore owns a second reference to each caller slot, so at commit time the refcount is 2 and the typed-ref escape check in typephp_helper.h rejects it.

The ArrayCleanupGuard that drops the aggregation array's references before the per-argument RefWrap::commit() lines was emitted too late: its cleanup() was appended after all commit lines, so every commit still observed refcount 2.

Scope matrix (all verified): fails for local-variable arguments (loop / non-loop, function / method, typed / untyped, named arguments); unaffected are property and array-element arguments (different wrapping paths) and non-variadic by-ref; literal arguments are a compile error, same as PHP.

Fix

Emit the guard when the aggregation array is created (inside the if ($variadicVar === null) block, after the per-statement unset()), so its cleanup() is queued in afterStmtLines before any RefWrap::commit() line, and drop the late emission at the statement tail. The guard stays per-call-site by design: ArrayCleanupGuard::cleanup() is one-shot (it clears its internal pointer; the destructor is only the exception-safety fallback).

Tests

New tests/compiler/variadic/by-reference-variadic-local.phpt covering, in one binary: typed local argument (non-loop and loop), named argument, untyped variadic, property argument, and array-element argument — with the expected output generated as ground truth against stock PHP 8.4.

Verification

  • tests/compiler/variadic: 14/14 pass (including fix(codegen): rebuild variadic aggregation array per statement #153's loop-accumulation.phpt and variadic-goto-skip.phpt)
  • Focused probe matrix (13 cases, incl. property / array-element / non-variadic guards): all green; literal by-ref argument still a compile error
  • Full tests/compiler: 1272 pass / 16 fail — the 16 failures are identical on master (pre-existing, unrelated areas: static binding, heredoc, enum, foreach, bigfloat, …)

Out of scope (pre-existing, separate)

A forward goto that skips a by-ref call fails C++ compilation on master today (crosses initialization of 'php::RefWrap<…>' / 'php::ArrayCleanupGuard' — call-site declarations live in the function body scope). This PR does not change that behavior (same state before and after); a follow-up will be tracked separately.

The aggregation php::Array holds a second reference to every caller
slot, but the ArrayCleanupGuard was emitted after all per-argument
RefWrap::commit() lines, so committing saw refcount 2 and raised
'A temporary typed reference cannot escape a dynamic call' for plain
local-variable arguments. Emit the guard when the aggregation array is
created so cleanup() is queued before the commit lines.

@matyhtf matyhtf 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.

The change fixes the reported variadic-only case, but the cleanup ordering still misses a fixed by-reference parameter sharing the same caller variable with a by-reference variadic parameter. Please address this case before merging.

Reproducer tested against PR HEAD 664d562b5516da9487cc4a2f76d218ac00831950:

function bump(&$first, &...$rest): void {
    $first += 10;
    foreach ($rest as &$value) {
        $value += 10;
    }
}

function main(): void {
    $a = 1;
    bump($a, $a);
    var_dump($a);
}

PHP prints int(21), while the PR build still throws:

A temporary typed reference cannot escape a dynamic call

The fixed argument is lowered before the variadic aggregation array is created. Its RefWrap::commit() has therefore already been queued when the new code appends cleanup(). The variadic argument reuses the same bridge, and the generated C++ still has this order:

php_bump(wrapper.ref(), args);
wrapper.commit();
guard.cleanup();

At commit(), the aggregation array still owns a reference, triggering the escape check. This case also fails on the base revision, so it is an uncovered case of the same root cause rather than a new regression.

Please:

  1. Arrange cleanup for each call so its reference aggregation array is released before all reference bridge commits belonging to that call, including bridges created for fixed parameters. Preserve argument evaluation order and exception cleanup.
  2. Add a PHPT regression for the shared-variable example above, alongside the existing loop and named-argument coverage.

Validation: all 14 variadic PHPTs and 107 related PHPUnit tests pass on this PR; the new PHPT fails on the base and passes on the PR. Additional repeated-alias and exception write-back probes also pass. The reproducer above is the remaining gap.

Thanks for the contribution and the regression test.

@yuan-dian
yuan-dian requested a review from matyhtf October 12, 2026 09:21
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.

2 participants