From 060ff694436c709f59975594f467c47ea5099cc1 Mon Sep 17 00:00:00 2001 From: mirchaemanuel Date: Tue, 7 Jul 2026 12:33:16 +0200 Subject: [PATCH] fix(codegen): callable-invoker trampoline preserves callee-saved registers Fixes #487. The generated descriptor-invoker trampoline saved only fp/lr and then used callee-saved registers as raw scratch (AArch64 x19-x26 via the nested-call register and the mixed/indexed argument loaders; x86_64 r12/rbx/r13-r15). The linear register allocator relies on the platform ABI and parks values that live across a callable_descriptor_invoke in exactly those registers, so any such value was clobbered by the invoke: in '$acc += $f()' inside a loop the loaded accumulator read back as the trampoline's leftover scratch (the empty args-array length, 0) and only the last call's value survived. Every indirect callable form was affected (closures, arrow functions, callable strings, first-class callables, method FCCs), with any operator, compound or not, including property targets; the closure body itself ran every iteration. Direct calls were safe because compiled functions save the allocator-used callee-saved registers in their own prologues - the invoker trampoline was the one generated path missing that treatment. --regalloc=stack masked the bug. The invoker frame grows to hold a save area and the prologue/epilogue now store/reload the callee-saved set it scratches, mirroring compiled-function prologues on both targets. Regression tests in tests/codegen/callables/expr_calls.rs (closure compound assign, callable-string/FCC with -= and *=, non-compound + property target). Broad suites green (2231 codegen + 993 error tests). --- CHANGELOG.md | 1 + src/codegen/runtime_callable_invoker.rs | 30 ++++++++++++- tests/codegen/callables/expr_calls.rs | 57 +++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b09eb3465a..a73cc6ab9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to elephc, a PHP-to-native compiler written in Rust. Releases are listed newest first. ## [Unreleased] +- Fixed a silent miscompile around indirect callable invocations (issue #487): the generated descriptor-invoker trampoline used callee-saved registers as scratch without preserving them, so any value the register allocator kept live across a closure / callable-string / first-class-callable call — most visibly the accumulator in `$acc += $f()` inside a loop — was clobbered and read back as the trampoline's leftover scratch. The invoker now saves and restores the callee-saved registers it touches, like every compiled function prologue (AArch64 x19–x26, x86_64 r12/rbx/r13–r15). - Removed the legacy direct AST → ASM backend completely. EIR is now the only codegen implementation path. - Int-backed enum `from()` / `tryFrom()` now accept a dynamically-typed (`mixed`) argument (issue #449): a `foreach` value over a heterogeneous array, an untyped parameter, etc. are coerced on their runtime type before the enum lookup — integer/numeric-string resolve (or throw `ValueError`), float truncates, bool/null coerce, and array/object/resource/closure throw `TypeError` naming the given type. Previously any `mixed` argument was rejected at compile time. Target-aware on every supported backend. diff --git a/src/codegen/runtime_callable_invoker.rs b/src/codegen/runtime_callable_invoker.rs index 5781dc6f9f..219475862a 100644 --- a/src/codegen/runtime_callable_invoker.rs +++ b/src/codegen/runtime_callable_invoker.rs @@ -25,7 +25,24 @@ use crate::types::{FunctionSig, PhpType}; const INVOKER_DESCRIPTOR_OFFSET: usize = 8; const INVOKER_CONCAT_OFFSET: usize = 16; -const INVOKER_FRAME_SIZE: usize = 32; +/// First frame slot of the callee-saved register save area (issue #487). +const INVOKER_SAVED_REGS_OFFSET: usize = 24; +/// Frame size covering the footer, descriptor/concat slots, and the callee-saved save +/// area (8 registers × 8 bytes on AArch64). +const INVOKER_FRAME_SIZE: usize = 96; + +/// Callee-saved registers the invoker body uses as scratch. The invoker is an ordinary +/// ABI function reached through `blr`/`call`, and the register allocator parks caller +/// values in these registers across `callable_descriptor_invoke` sites, so the trampoline +/// must preserve them like any compiled function prologue does (issue #487): without the +/// save/restore, a value such as the loaded LHS of `$acc += $f()` read back as the +/// trampoline's leftover scratch (the args-array length) after every indirect call. +fn invoker_saved_callee_regs(arch: Arch) -> &'static [&'static str] { + match arch { + Arch::AArch64 => &["x19", "x20", "x21", "x22", "x23", "x24", "x25", "x26"], + Arch::X86_64 => &["r12", "rbx", "r13", "r14", "r15"], + } +} /// Runtime invoker metadata emitted beside callable descriptors. pub(super) struct RuntimeCallableInvoker<'a> { @@ -85,6 +102,12 @@ pub(super) fn emit_runtime_callable_invoker( emitter.raw(".align 2"); emitter.label_global(invoker.label); abi::emit_frame_prologue(emitter, INVOKER_FRAME_SIZE); + // Preserve the caller's callee-saved registers before the body scratches them: the + // register allocator keeps values live across the indirect invoke in these registers + // (issue #487). + for (index, reg) in invoker_saved_callee_regs(emitter.target.arch).iter().enumerate() { + abi::store_at_offset(emitter, reg, INVOKER_SAVED_REGS_OFFSET + index * 8); + } abi::store_at_offset( emitter, abi::int_arg_reg_name(emitter.target, 0), @@ -103,6 +126,11 @@ pub(super) fn emit_runtime_callable_invoker( data, ); emit_box_current_value_as_mixed(emitter, &ret_ty.codegen_repr()); + // Restore the caller's callee-saved registers (the boxed result travels in the + // return registers, which the restore loads never touch). + for (index, reg) in invoker_saved_callee_regs(emitter.target.arch).iter().enumerate() { + abi::load_at_offset(emitter, reg, INVOKER_SAVED_REGS_OFFSET + index * 8); + } abi::emit_frame_restore(emitter, INVOKER_FRAME_SIZE); abi::emit_return(emitter); } diff --git a/tests/codegen/callables/expr_calls.rs b/tests/codegen/callables/expr_calls.rs index 6ebba7be1f..30f7479234 100644 --- a/tests/codegen/callables/expr_calls.rs +++ b/tests/codegen/callables/expr_calls.rs @@ -1451,3 +1451,60 @@ echo $cb(); ); assert_eq!(out, "B"); } + +/// Regression for #487: a value the register allocator parks in a callee-saved register +/// across an indirect callable invoke (here the loaded LHS of `$acc += $f()`) must survive +/// the call. The generated descriptor-invoker trampoline used callee-saved registers as +/// scratch without saving them, so the accumulator read back as the trampoline's leftover +/// (the empty args-array length, 0) and only the last call's value survived. +#[test] +fn test_compound_assign_closure_call_rhs_accumulates() { + let out = compile_and_run( + r#"p += $f(); +} +echo $acc, "|", $b->p; +"#, + ); + assert_eq!(out, "80|80"); +}