diff --git a/docs/compiling/output-and-diagnostics.md b/docs/compiling/output-and-diagnostics.md index e09bcdb5c2..8edaeaa743 100644 --- a/docs/compiling/output-and-diagnostics.md +++ b/docs/compiling/output-and-diagnostics.md @@ -118,6 +118,10 @@ elephc --gc-stats heavy.php ./heavy ``` +Combined with `--web`, the server never reaches the process-exit report, so the +counters are printed to stderr after every handled request instead — a growing +`allocs - frees` gap across requests indicates a per-request leak. + ### `--heap-debug` Enables runtime heap verification in the compiled program: double-free diff --git a/src/codegen/frame.rs b/src/codegen/frame.rs index cb9b2cc2cd..9fb0aec814 100644 --- a/src/codegen/frame.rs +++ b/src/codegen/frame.rs @@ -356,12 +356,19 @@ pub(super) fn emit_web_handler_prologue(ctx: &mut FunctionContext<'_>) { /// /// Like `emit_main_epilogue` it runs the per-request main local cleanup (so /// owned refcounted top-level locals are released each request) and restores the -/// frame, but it `ret`s instead of exiting and skips the process-end gc-stats and -/// heap-debug diagnostics, which are wrong to report per request. +/// frame, but it `ret`s instead of exiting. Requested gc-stats are emitted after +/// cleanup for every request; process-end heap-debug diagnostics remain skipped. pub(super) fn emit_web_handler_epilogue(ctx: &mut FunctionContext<'_>) { ctx.emitter.blank(); ctx.emitter.comment("web handler epilogue + ret"); emit_main_local_epilogue_cleanup(ctx); + // Under `--web` the handler returns to the bridge server loop instead of + // exiting, so the exit-based main epilogue (where `--gc-stats` normally + // prints) is never reached. Emitting the counters here, once per request, + // is the only way to observe them in server mode. + if ctx.gc_stats { + emit_gc_stats(ctx); + } emit_callee_saved_restores(ctx); abi::emit_frame_restore(ctx.emitter, ctx.frame_size); abi::emit_return(ctx.emitter); diff --git a/src/ir_lower/context.rs b/src/ir_lower/context.rs index c85894163d..d51a18deaa 100644 --- a/src/ir_lower/context.rs +++ b/src/ir_lower/context.rs @@ -1543,6 +1543,36 @@ impl<'m, 'f> LoweringContext<'m, 'f> { && matches!(op, Some(Op::ArrayGet | Op::HashGet)) } + /// Returns whether an index-read receiver is itself an owned intermediate + /// produced by an index read, i.e. the inner step of a chained subscript + /// read such as `$a[$i][$j]`. + /// + /// Container reads of refcounted or boxed-Mixed elements return a +1 caller + /// reference (the `array_get`/`hash_get` emitters incref pointer payloads and + /// box Mixed cells), so when that result is consumed directly as the receiver + /// of another index read there is no local slot whose release machinery would + /// ever drop the reference — the consuming read must release it explicitly. + /// String results are excluded: they are borrowed pointers into the container + /// payload and carry no reference of their own. + pub(crate) fn value_is_owned_index_read_temp(&self, value: LoweredValue) -> bool { + let php_type = self.builder.value_php_type(value.value).codegen_repr(); + if !(matches!(php_type, PhpType::Mixed | PhpType::Union(_)) + || (php_type.is_refcounted() && php_type != PhpType::Str)) + { + return false; + } + matches!( + self.builder.value_defining_op(value.value), + Some( + Op::ArrayGet + | Op::ArrayGetSilent + | Op::HashGet + | Op::ArrayGetMixedKey + | Op::ArrayGetMixedKeySilent + ) + ) + } + /// Returns true for builtin calls whose return value is newly allocated for the caller. fn value_is_owning_builtin_temporary(&self, value: ValueId) -> bool { let Some(inst) = self.builder.value_defining_instruction(value) else { diff --git a/src/ir_lower/expr/mod.rs b/src/ir_lower/expr/mod.rs index a4cf283cf5..0d7403d15b 100644 --- a/src/ir_lower/expr/mod.rs +++ b/src/ir_lower/expr/mod.rs @@ -8593,14 +8593,38 @@ fn lower_array_access_from_value( _ => Op::RuntimeCall, }; let result_type = array_access_result_type(ctx, array_value.value, op, expr); - ctx.emit_value( + let release_receiver = ctx.value_is_owned_index_read_temp(array_value); + let mut result = ctx.emit_value( op, vec![array_value.value, index_value.value], None, result_type, op.default_effects(), Some(expr.span), - ) + ); + // A chained subscript read (`$a[$i][$j]`) consumes the inner read's result + // directly as its receiver. That intermediate carries a +1 reference (the + // get emitters incref refcounted payloads and box Mixed cells), and unlike + // the hoisted form `$inner = $a[$i]; $inner[$j]` no local-slot release + // machinery ever drops it. Typed string reads borrow their bytes from that + // receiver, so stabilize the leaf before releasing the receiver: a later + // expression can otherwise drop the parent's last reference and leave the + // extracted string dangling. Mixed reads already return independent owned + // cells and do not need this copy. + if release_receiver && result.ir_type == IrType::Str { + result = ctx.emit_value( + Op::StrPersist, + vec![result.value], + None, + PhpType::Str, + Op::StrPersist.default_effects(), + Some(expr.span), + ); + } + if release_receiver { + crate::ir_lower::ownership::release_if_owned(ctx, array_value, Some(expr.span)); + } + result } /// Lowers nullable receiver indexing without evaluating the index on a null receiver. @@ -8929,6 +8953,12 @@ fn lower_ternary( /// Lowers a cast expression. fn lower_cast(ctx: &mut LoweringContext<'_, '_>, target: &CastType, inner: &Expr, expr: &Expr) -> LoweredValue { let value = lower_expr(ctx, inner); + // Keep the original producer visible for a no-op string cast. Wrapping an + // owned string temporary in `Cast(Str)` would hide its ownership from the + // retaining store/call cleanup and leak the detached string allocation. + if matches!(target, CastType::String) && value.ir_type == IrType::Str { + return value; + } let php_type = cast_php_type(target); let result = ctx.emit_value( Op::Cast, diff --git a/tests/codegen/eval.rs b/tests/codegen/eval.rs index c9fc08152a..8009e26b26 100644 --- a/tests/codegen/eval.rs +++ b/tests/codegen/eval.rs @@ -19832,32 +19832,63 @@ eval('enum bool { case Ready; }'); ); } -/// Verifies eval rejects PHP-reserved bare class-like reference names. +/// Asserts one eval fragment rejects a PHP-reserved bare class-like reference name. +fn assert_eval_reserved_class_like_reference_name_fails(source: &str) { + let err = compile_and_run_expect_failure(source); + assert!( + err.contains("Fatal error: eval() fragment uses an unsupported construct"), + "stderr did not contain eval unsupported-construct diagnostic: {err}" + ); +} + +/// Verifies eval rejects a reserved class name in an `extends` reference. #[test] -fn test_eval_reserved_class_like_reference_name_fails() { - for source in [ +fn test_eval_reserved_extends_class_reference_name_fails() { + assert_eval_reserved_class_like_reference_name_fails( r#"= frees && allocs - frees < 10, + "chained three-level reads must release their intermediates: allocs={} frees={}", + allocs, + frees + ); +} + +/// Regression test for issue #516 (string-keyed nesting): a chained +/// associative read (`$m['x']['y']`) goes through `hash_get`, whose refcounted +/// results also carry a +1 caller reference. The chained consumer must release +/// the intermediate hash exactly like the indexed-array path. Regression: heap +/// must be clean at exit after repeated evaluation. +#[test] +fn test_regression_516_chained_string_keyed_read_does_not_leak() { + let out = compile_and_run_with_heap_debug( + r#" ['y' => 'deep', 'z' => 'other'], 'w' => ['y' => 'second']]; +for ($i = 0; $i < 200; $i++) { + $v = (string) $m['x']['y']; +} +echo $m['x']['y']; +"#, + ); + assert!(out.success, "program failed: {}", out.stderr); + assert_eq!(out.stdout, "deep"); + assert!( + out.stderr.contains("HEAP DEBUG: leak summary: clean"), + "expected a clean heap, got: {}", + out.stderr + ); +} + +/// Regression test for issue #516: a typed string extracted from a chained +/// read must remain valid when a later call argument releases the parent array. +#[test] +fn test_regression_516_chained_string_survives_parent_release() { + let out = compile_and_run_with_heap_debug( + r#" String { }) } -/// Compiles `source` with the given extra elephc flags; returns the binary path. -fn compile_web(dir: &Path, source: &str, stem: &str) -> PathBuf { +/// Compiles `source` in web mode with extra compiler flags and returns the binary path. +fn compile_web_with_flags(dir: &Path, source: &str, stem: &str, flags: &[&str]) -> PathBuf { let php = dir.join(format!("{}.php", stem)); fs::write(&php, source).unwrap(); let mut cmd = Command::new(elephc_bin()); cmd.env("XDG_CACHE_HOME", dir.join("cache-root")); cmd.current_dir(dir); - cmd.arg("--web").arg(&php); + cmd.arg("--web").args(flags).arg(&php); let output = cmd.output().expect("failed to spawn elephc"); assert!( output.status.success(), @@ -62,6 +62,11 @@ fn compile_web(dir: &Path, source: &str, stem: &str) -> PathBuf { dir.join(stem) } +/// Compiles `source` in web mode without extra compiler flags. +fn compile_web(dir: &Path, source: &str, stem: &str) -> PathBuf { + compile_web_with_flags(dir, source, stem, &[]) +} + /// Picks an ephemeral localhost port by binding :0 and releasing it. fn free_port() -> u16 { let l = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); @@ -578,6 +583,60 @@ fn web_conditional_early_return_halts() { assert!(good.ends_with("good"), "ok body must be 'good': {:?}", good); } +/// Verifies `--gc-stats` emits one counter line after every web request, +/// including a request that leaves the top-level handler through early return. +#[test] +fn web_gc_stats_are_emitted_per_request() { + let dir = make_test_dir("web_gc_stats"); + let src = " = stderr + .lines() + .filter(|line| line.starts_with("GC: allocs=")) + .collect(); + assert_eq!( + stats.len(), + 2, + "expected one gc-stats line per handled request, stderr: {}", + stderr + ); + for line in stats { + let Some((allocs, frees)) = line + .strip_prefix("GC: allocs=") + .and_then(|rest| rest.split_once(" frees=")) + else { + panic!("malformed gc-stats line: {}", line); + }; + assert!(allocs.parse::().is_ok(), "invalid allocation count: {}", line); + assert!(frees.parse::().is_ok(), "invalid free count: {}", line); + } +} + /// Verifies a request body over --max-body-size is rejected with 413, and a body /// under the limit is served normally. #[test]