Language: give the exact arg->producer link the last word (#23354) - #23424
Conversation
Follow-up to the argument-mapping fix. Four more sites resolved a hoisted call
argument from the statement immediately before the call — only ever the trailing
argument's producer — and applied it to an index it does not belong to:
* resolveInlineArrayLiteralDimFetchCallArgSlot() returned children[$callIndex - 1]
for any $argIndex, with no ordinal check at all.
* resolvePrecedingExpressionPreludeCallArgSlot() is hardcoded to $argIndex === 0
while reading the trailing producer — exactly backwards on a multi-arg call.
* resolvePrecedingArrayDimFetchCallArgSlot() aligned fewer-fetches-than-args to
the FIRST non-embedded arguments; hoisted fetches sit immediately before the
call and so belong to the LAST ones.
* inlineHoistedProducerForCallArgIndex() fell back to positional mapping.
Rather than add a sixth positional heuristic, php-cfg's exact link now has the last
word before ARG_SEND is emitted: the hoisted argument temporary is a distinct Operand
from the producer's ->result but records that producer as its sole writer, so
args[$i]->ops[0] is right by construction rather than by shape. Restricted to dead
inline temporaries with exactly one writer whose producer is a hoisted statement of
this block before the call; everything else keeps the existing paths.
Underneath sat a second defect. Block::getFrame() builds a fresh scope array per
block and findVariableInParentFrames() resolves by NAME, but temporaries have none
(deliberately, #3790) — so every temporary computed before a CFG block split read
back empty afterwards, which is why f($x + 1, $r['k']) stayed wrong even once the
mapping was right. Every other continuation block (CFG merges, try/catch bodies,
short-circuit arms) sets inheritUndefinedLocals so getFrame() inherits the parent
frame's slots; the continuation from splitCfgBlockAfterStringKeyedArray() did not.
Differential sweep against Zend over the same 43 programs: 24 mismatching on master,
8 after the first fix, 0 now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regression verification complete — all three candidates confirmed not caused by this change:
All three fail on Net: 0 regressions, 23 fixed across 7,387 comparable cases. A full |
Maintainer verify + merge (2026-07-26)Host Zend 8.2.32 vs
Merged as (#10533 comments locked at 2500+) Gate note for this maintainer run: |
The compliance suite asserts against recorded expectations, so it only catches what someone already thought to record. Nothing in the tree compared the compiler's output against Zend's on arbitrary programs, which is why #23354 — multi-argument calls handing every argument the trailing one's value — survived with no diagnostic: f($x + 1, $x + 2) printed "12 12" and str_replace($p['from'], $p['to'], 'xy!') returned "xy!", both running happily to completion. 24 of these 43 programs mismatched Zend on master when the corpus was written. All are fixed (#23356, #23424); the corpus stays as the regression guard, and the harness generalises to any directory of programs via --dir. Cases are deliberately mundane — multi-argument calls by producer kind, mixed producer kinds in one call, and the shapes the argument-resolution heuristics were individually tuned against (var_export, usort, in_array, array_merge, sprintf, by-ref, named args, spread, closures, static calls, constructor promotion). Exit status is the mismatch count so it can gate a build. Co-authored-by: PurHur <tedyyyyy@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes #23354. Stacked on #23356 — review that one first.
#23356 fixed two of the sites that hand a hoisted call argument the trailing argument's producer. Tracing the rest of the corpus turned up four more copies of the same mistake, plus a second, unrelated defect underneath them.
Four more of the same
resolveInlineArrayLiteralDimFetchCallArgSlot()children[$callIndex - 1]for any$argIndex— no ordinal check at allresolvePrecedingExpressionPreludeCallArgSlot()0 === $argIndexwhile reading the trailing producer — exactly backwards on a multi-argument callresolvePrecedingArrayDimFetchCallArgSlot()inlineHoistedProducerForCallArgIndex()Rather than add a sixth positional heuristic, php-cfg's exact link now gets the last word before
ARG_SENDis emitted. The hoisted argument temporary is a distinctOperandfrom the producer's->result— which is whyslotForOperand($arg)misses and all of these heuristics exist — but it records that producer as its sole writer, soargs[$i]->ops[0]is right by construction rather than by shape. Restricted to dead inline temporaries with exactly one writer whose producer is a hoisted statement of this block before the call; everything else keeps the existing paths.The defect underneath
f($x + 1, $r['k'])still printedK|Kafter all of those, because the value never reaches the call at all.Block::getFrame()builds a fresh scope array per block. Named variables survive a CFG edge viafindVariableInParentFrames()— but that resolves by name, and temporaries have none (deliberately, #3790). So every temporary computed before a block split reads back empty afterwards.['k' => 'K']triggerssplitCfgBlockAfterStringKeyedArray(), and its continuation was the only one in the compiler that does not setinheritUndefinedLocals, which is what makesgetFrame()inherit the parent frame's slots:CFG merges, try/catch bodies and short-circuit arms all already set it.
Validation
Differential execution against Zend over the same 43 programs as #23356:
Adds
call_arg_producer_mixed.phpt, covering mixed producers, trailing property fetch, the split-stranded case, ternary arms, independent nested dim chains, and comparison producers. It fails on master (K|…) and on #23356 alone (K|K…); only this change makes it pass.Compliance
VMTest(7,549 cases) run base-vs-branch; results in the thread.Results
Differential execution against Zend, rebased onto current
master:Compliance
VMTest, run on this branch and onmaster, compared by failing case name (the suite is not green onmaster— 407 pre-existing failures), sharded 24 ways with per-test TeamCity output:Three cases first appeared as regressions and were each checked individually rather than assumed:
language/interface_abstract_static_call,types/dnf_return_type_error,stdlib/proc_get_status_basic— all three fail 5/5 on master as well when run in isolation, and re-running the shard that contains it flippedinterface_abstract_static_callto failing onmastertoo. They are order-dependent, not caused by this change.proc_get_status_basicis the clearest case: it appeared on the fixed list in the earlier Language: map each hoisted call arg to its own producer (#23354) #23356 comparison and on the regressed list here.The 23 fixed cases are dominated by call-argument shapes —
in_array/array_searchwith enum arguments (7 cases),array_udiff_enum,array_multisort_byref_call_flags,call_user_func_array_class_string_inline,sscanf_array_element_ref,date_sunrise_sunset_nested_strtotime,password_needs_rehash_nested_hash— which is what the exact-link mapping is expected to repair.