Skip to content

Commit 0ab3940

Browse files
feat: persistent-zval helpers (deep-copy zval trees across threads)
Second step of the split suggested in #2287: land the persistent-zval subsystem as a standalone, reviewable header, independent of background workers. This is the subsystem most likely to hide latent refcount or memory-lifetime bugs; reviewing it in isolation is higher-signal than finding issues inside a 3k-line diff. - persistent_zval.h (renamed from the bg_worker_vars.h draft, prefix dropped for generality): - persistent_zval_validate: whitelist (scalars, arrays of allowed values, enum instances). Everything else fails fast. - persistent_zval_persist: deep-copy request -> persistent (pemalloc) memory. Fast paths baked in: interned strings shared, opcache- immutable arrays passed by pointer without copying or owning. - persistent_zval_free: deep-free; skips interned strings and immutable arrays (borrowed, not owned). - persistent_zval_to_request: deep-copy persistent -> fresh request memory. Enums re-resolved by class + case name on each read. - frankenphp.c: header included only when FRANKENPHP_TEST_HOOKS is defined. First real consumer (background workers) drops the guard. - Test hook gated on FRANKENPHP_TEST_HOOKS: - PHP function frankenphp_test_persist_roundtrip(mixed): mixed runs validate -> persist -> to_request -> free and returns the result. - Registered via zend_register_functions at MINIT so it never appears in ext_functions[] and never ships in production builds. - CI workflows set -DFRANKENPHP_TEST_HOOKS in CGO_CFLAGS (tests.yaml + sanitizers.yaml). windows.yaml is the release build, not a test runner, and stays untouched. - Build verified both without the flag (production path, no unused-function warnings) and with it (test path). - The FRANKENPHP_TEST_HOOKS guard around the header include goes away in the PR that lands the first real caller; the test hook itself goes away in that same step once end-to-end tests cover the code paths.
1 parent cc8dfa8 commit 0ab3940

6 files changed

Lines changed: 468 additions & 4 deletions

File tree

.github/workflows/sanitizers.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ jobs:
108108
- name: Set CGO flags
109109
run: |
110110
{
111-
echo "CGO_CFLAGS=$CFLAGS -I${PWD}/watcher/target/include $(php-config --includes)"
111+
echo "CGO_CFLAGS=$CFLAGS -I${PWD}/watcher/target/include -DFRANKENPHP_TEST $(php-config --includes)"
112112
echo "CGO_LDFLAGS=$LDFLAGS $(php-config --ldflags) $(php-config --libs)"
113113
} >> "$GITHUB_ENV"
114114
- name: Run tests

.github/workflows/tests.yaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ jobs:
5959
- name: Install e-dant/watcher
6060
uses: ./.github/actions/watcher
6161
- name: Set CGO flags
62-
run: echo "CGO_CFLAGS=-I${PWD}/watcher/target/include $(php-config --includes)" >> "${GITHUB_ENV}"
62+
run: echo "CGO_CFLAGS=-I${PWD}/watcher/target/include -DFRANKENPHP_TEST $(php-config --includes)" >> "${GITHUB_ENV}"
6363
- name: Build
6464
run: go build
6565
- name: Build testcli binary
@@ -135,7 +135,7 @@ jobs:
135135
echo "GEN_STUB_SCRIPT=${PWD}/php-${PHP_VERSION}/build/gen_stub.php" >> "${GITHUB_ENV}"
136136
- name: Set CGO flags
137137
run: |
138-
echo "CGO_CFLAGS=$(php-config --includes)" >> "${GITHUB_ENV}"
138+
echo "CGO_CFLAGS=-DFRANKENPHP_TEST $(php-config --includes)" >> "${GITHUB_ENV}"
139139
echo "CGO_LDFLAGS=$(php-config --ldflags) $(php-config --libs)" >> "${GITHUB_ENV}"
140140
- name: Install gotestsum
141141
run: go install gotest.tools/gotestsum@latest
@@ -172,7 +172,7 @@ jobs:
172172
- name: Set CGO flags
173173
run: |
174174
{
175-
echo "CGO_CFLAGS=-I/opt/homebrew/include/ $(php-config --includes)"
175+
echo "CGO_CFLAGS=-I/opt/homebrew/include/ -DFRANKENPHP_TEST $(php-config --includes)"
176176
echo "CGO_LDFLAGS=-L/opt/homebrew/lib/ $(php-config --ldflags) $(php-config --libs)"
177177
} >> "${GITHUB_ENV}"
178178
- name: Build

frankenphp.c

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@
3737

3838
#include "_cgo_export.h"
3939
#include "frankenphp_arginfo.h"
40+
#ifdef FRANKENPHP_TEST
41+
/* The persistent_zval helpers are only compiled in when a consumer needs
42+
* them. The step that lands the first real caller (background workers)
43+
* will drop this guard. */
44+
#include "zval.h"
45+
#endif
4046

4147
#if defined(PHP_WIN32) && defined(ZTS)
4248
ZEND_TSRMLS_CACHE_DEFINE()
@@ -712,12 +718,55 @@ PHP_FUNCTION(frankenphp_log) {
712718
}
713719
}
714720

721+
#ifdef FRANKENPHP_TEST
722+
/* Test-only entry point that exercises zval.h end-to-end:
723+
* validate -> persist (request -> persistent memory) ->
724+
* to_request (persistent -> fresh request memory) -> free persistent copy.
725+
* Compiled only when FRANKENPHP_TEST is defined; never registered
726+
* in production builds. */
727+
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(
728+
arginfo_frankenphp_test_persist_roundtrip, 0, 1, IS_MIXED, 0)
729+
ZEND_ARG_TYPE_INFO(0, value, IS_MIXED, 0)
730+
ZEND_END_ARG_INFO()
731+
732+
PHP_FUNCTION(frankenphp_test_persist_roundtrip) {
733+
zval *input;
734+
ZEND_PARSE_PARAMETERS_START(1, 1)
735+
Z_PARAM_ZVAL(input)
736+
ZEND_PARSE_PARAMETERS_END();
737+
738+
if (!persistent_zval_validate(input)) {
739+
zend_throw_exception(spl_ce_LogicException,
740+
"persistent_zval: value type not supported "
741+
"(only scalars, arrays, and enums are allowed)",
742+
0);
743+
RETURN_THROWS();
744+
}
745+
746+
zval persistent;
747+
persistent_zval_persist(&persistent, input);
748+
persistent_zval_to_request(return_value, &persistent);
749+
persistent_zval_free(&persistent);
750+
}
751+
752+
static const zend_function_entry frankenphp_test_hook_functions[] = {
753+
PHP_FE(frankenphp_test_persist_roundtrip,
754+
arginfo_frankenphp_test_persist_roundtrip) PHP_FE_END};
755+
#endif
756+
715757
PHP_MINIT_FUNCTION(frankenphp) {
716758
register_frankenphp_symbols(module_number);
717759
#ifndef PHP_WIN32
718760
pthread_atfork(NULL, NULL, frankenphp_fork_child);
719761
#endif
720762

763+
#ifdef FRANKENPHP_TEST
764+
if (zend_register_functions(NULL, frankenphp_test_hook_functions, NULL,
765+
MODULE_PERSISTENT) == FAILURE) {
766+
return FAILURE;
767+
}
768+
#endif
769+
721770
zend_function *func;
722771

723772
// Override putenv

testdata/persist-roundtrip.php

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
<?php
2+
3+
// Exercises frankenphp_test_persist_roundtrip (only registered when
4+
// FRANKENPHP_TEST_HOOKS is defined at build time). The script runs as a
5+
// plain non-worker request; the Go harness asserts the combined output.
6+
7+
enum Status: string {
8+
case Active = 'active';
9+
case Paused = 'paused';
10+
}
11+
12+
function same(mixed $actual, mixed $expected, string $label): void {
13+
if ($actual !== $expected) {
14+
echo "FAIL $label: expected ";
15+
var_export($expected);
16+
echo ' got ';
17+
var_export($actual);
18+
echo "\n";
19+
return;
20+
}
21+
echo "OK $label\n";
22+
}
23+
24+
$rt = 'frankenphp_test_persist_roundtrip';
25+
if (!function_exists($rt)) {
26+
echo "SKIP frankenphp_test_persist_roundtrip not registered\n";
27+
return;
28+
}
29+
30+
// Scalars.
31+
same($rt(null), null, 'null');
32+
same($rt(false), false, 'false');
33+
same($rt(true), true, 'true');
34+
same($rt(0), 0, 'int zero');
35+
same($rt(42), 42, 'int');
36+
same($rt(-1), -1, 'int negative');
37+
same($rt(1.5), 1.5, 'float');
38+
same($rt(''), '', 'empty string');
39+
same($rt('hello'), 'hello', 'short string');
40+
41+
// Long (non-interned) string: forces the allocation path.
42+
$long = str_repeat('x', 1024);
43+
same($rt($long), $long, 'long string');
44+
45+
// Nested arrays, mixed keys.
46+
$arr = [
47+
'name' => 'alice',
48+
'age' => 30,
49+
'tags' => ['admin', 'editor'],
50+
'meta' => ['created' => 1234567890, 'flags' => [true, false, null]],
51+
0 => 'first',
52+
1 => 'second',
53+
];
54+
same($rt($arr), $arr, 'nested array');
55+
56+
// Enum roundtrip: identity (===) must be preserved because the enum is
57+
// re-resolved to the same singleton case on the read side.
58+
same($rt(Status::Active), Status::Active, 'enum active');
59+
same($rt(Status::Paused), Status::Paused, 'enum paused');
60+
61+
// Array containing an enum.
62+
same(
63+
$rt(['status' => Status::Active, 'count' => 7]),
64+
['status' => Status::Active, 'count' => 7],
65+
'array with enum',
66+
);
67+
68+
// Invalid inputs throw LogicException.
69+
try {
70+
$rt(new stdClass());
71+
echo "FAIL stdClass should throw\n";
72+
} catch (\LogicException) {
73+
echo "OK stdClass rejected\n";
74+
}
75+
76+
try {
77+
$rt(fopen('php://memory', 'r'));
78+
echo "FAIL resource should throw\n";
79+
} catch (\LogicException) {
80+
echo "OK resource rejected\n";
81+
}
82+
83+
try {
84+
$rt(['ok' => 1, 'bad' => new stdClass()]);
85+
echo "FAIL nested stdClass should throw\n";
86+
} catch (\LogicException) {
87+
echo "OK nested stdClass rejected\n";
88+
}

0 commit comments

Comments
 (0)