Skip to content

Commit bf4b926

Browse files
committed
Merge branch 'PHP-8.4' into PHP-8.5
* PHP-8.4: CS Fix phpGH-22857: Function JIT emits wrong code for FETCH_OBJ_FUNC_ARG on a property hook (php#22897)
2 parents 5ee37f6 + 40c3e24 commit bf4b926

5 files changed

Lines changed: 253 additions & 5 deletions

File tree

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ PHP NEWS
1717
(David Carlier)
1818
. Fixed bug GH-22763 (JIT fails to clear ZREG_TYPE_ONLY after setting reg).
1919
(Arnaud)
20+
. Fixed bug GH-22857 (Function JIT emits wrong code for FETCH_OBJ_FUNC_ARG on
21+
a property hook getter, losing register-held variables). (Zhao Hao)
2022

2123
- MBString:
2224
. Fixed bug GH-22779 (mb_strrpos() returns the wrong position for a negative

ext/opcache/jit/zend_jit.c

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2446,6 +2446,7 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
24462446
goto jit_failure;
24472447
}
24482448
goto done;
2449+
case ZEND_FETCH_OBJ_FUNC_ARG:
24492450
case ZEND_FETCH_OBJ_R:
24502451
case ZEND_FETCH_OBJ_IS:
24512452
case ZEND_FETCH_OBJ_W:
@@ -2483,11 +2484,31 @@ static int zend_jit(const zend_op_array *op_array, zend_ssa *ssa, const zend_op
24832484
|| Z_STRVAL_P(RT_CONSTANT(opline, opline->op2))[0] == '\0') {
24842485
break;
24852486
}
2486-
if (!zend_jit_fetch_obj(&ctx, opline, op_array, ssa, ssa_op,
2487-
op1_info, op1_addr, 0, ce, ce_is_instanceof, on_this, 0, 0, NULL,
2488-
RES_REG_ADDR(), IS_UNKNOWN,
2489-
zend_may_throw(opline, ssa_op, op_array, ssa))) {
2490-
goto jit_failure;
2487+
if (opline->opcode == ZEND_FETCH_OBJ_FUNC_ARG) {
2488+
/* FETCH_OBJ_FUNC_ARG's by-value fetch dispatches into the
2489+
* FETCH_OBJ_R handler, which may take the SIMPLE_GET hook fast
2490+
* path and push a getter frame; by-ref dispatches into
2491+
* FETCH_OBJ_W. The function JIT may keep values solely in
2492+
* registers, so we must NOT exit to the VM (stale stack slots).
2493+
* Inline the by-value path through zend_jit_fetch_obj, which runs
2494+
* the hook getter inside a helper and keeps all registers live.
2495+
* The by-ref path has no SIMPLE_GET fast path, so the generic
2496+
* handler (a full C call, safe under register allocation) is used.
2497+
* This mirrors the tracing JIT fix for GH-21006 (GH-21369); the
2498+
* runtime by-ref check is required because the passing mode is
2499+
* only known once the callee is resolved via namespace fallback.
2500+
* See GH-22857. */
2501+
if (!zend_jit_fetch_obj_func_arg(jit, opline, op_array, ssa, ssa_op,
2502+
op1_info, op1_addr, ce, ce_is_instanceof, on_this, RES_REG_ADDR())) {
2503+
goto jit_failure;
2504+
}
2505+
} else {
2506+
if (!zend_jit_fetch_obj(&ctx, opline, op_array, ssa, ssa_op,
2507+
op1_info, op1_addr, 0, ce, ce_is_instanceof, on_this, 0, 0, NULL,
2508+
RES_REG_ADDR(), IS_UNKNOWN,
2509+
zend_may_throw(opline, ssa_op, op_array, ssa))) {
2510+
goto jit_failure;
2511+
}
24912512
}
24922513
goto done;
24932514
case ZEND_FETCH_STATIC_PROP_R:

ext/opcache/jit/zend_jit_ir.c

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14736,6 +14736,59 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit,
1473614736
return 1;
1473714737
}
1473814738

14739+
static int zend_jit_fetch_obj_func_arg(zend_jit_ctx *jit, const zend_op *opline,
14740+
const zend_op_array *op_array, zend_ssa *ssa, const zend_ssa_op *ssa_op,
14741+
uint32_t op1_info, zend_jit_addr op1_addr, zend_class_entry *ce,
14742+
bool ce_is_instanceof, bool on_this, zend_jit_addr res_addr)
14743+
{
14744+
ir_ref rx, call_info, if_by_ref, end_by_ref;
14745+
14746+
/* Both runtime paths must observe a consistent frame state. The delayed
14747+
* call chain would otherwise only be flushed inside the by-ref branch (by
14748+
* zend_jit_set_ip() in zend_jit_handler()), leaving EX(call) stale on the
14749+
* by-val path and after the merge. Flush it before branching. */
14750+
if (jit->delayed_call_level) {
14751+
if (!zend_jit_save_call_chain(jit, jit->delayed_call_level)) {
14752+
return 0;
14753+
}
14754+
}
14755+
14756+
/* JIT: if (ZEND_CALL_INFO(EX(call)) & ZEND_CALL_SEND_ARG_BY_REF) */
14757+
if (jit->reuse_ip) {
14758+
rx = jit_IP(jit);
14759+
} else {
14760+
rx = ir_LOAD_A(jit_EX(call));
14761+
}
14762+
call_info = ir_LOAD_U32(jit_CALL(rx, This.u1.type_info));
14763+
if_by_ref = ir_IF(ir_AND_U32(call_info, ir_CONST_U32(ZEND_CALL_SEND_ARG_BY_REF)));
14764+
14765+
/* by-ref path: the FUNC_ARG handler re-checks the flag and dispatches
14766+
* into FETCH_OBJ_W */
14767+
ir_IF_TRUE_cold(if_by_ref);
14768+
if (!zend_jit_handler(jit, opline, zend_may_throw(opline, ssa_op, op_array, ssa))) {
14769+
return 0;
14770+
}
14771+
end_by_ref = ir_END();
14772+
14773+
/* zend_jit_handler() stored IP = opline + 1 on the by-ref path only;
14774+
* that compile-time knowledge is invalid for the by-val path and after
14775+
* the merge. */
14776+
zend_jit_reset_last_valid_opline(jit);
14777+
14778+
/* by-val path */
14779+
ir_IF_FALSE(if_by_ref);
14780+
if (!zend_jit_fetch_obj(jit, opline, op_array, ssa, ssa_op,
14781+
op1_info, op1_addr, 0, ce, ce_is_instanceof, on_this, 0, 0, NULL,
14782+
res_addr, IS_UNKNOWN,
14783+
zend_may_throw(opline, ssa_op, op_array, ssa))) {
14784+
return 0;
14785+
}
14786+
ir_MERGE_WITH(end_by_ref);
14787+
14788+
return 1;
14789+
}
14790+
14791+
1473914792
static int zend_jit_assign_obj(zend_jit_ctx *jit,
1474014793
const zend_op *opline,
1474114794
const zend_op_array *op_array,

ext/opcache/tests/jit/gh22857.phpt

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
--TEST--
2+
GH-22857: Function JIT emits wrong code for FETCH_OBJ_FUNC_ARG on a property hook (SIMPLE_GET fast path)
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
opcache.jit_buffer_size=64M
7+
opcache.jit=1205
8+
opcache.jit_hot_func=1
9+
--EXTENSIONS--
10+
opcache
11+
--FILE--
12+
<?php
13+
namespace Test;
14+
15+
interface HandlerInterface { public function noop(): void; }
16+
17+
final class DefaultHandler implements HandlerInterface {
18+
private static ?self $i = null;
19+
public static function getInstance(): self { return self::$i ??= new self(); }
20+
public function noop(): void {}
21+
}
22+
23+
/* Original issue: virtual property hook read via FETCH_OBJ_FUNC_ARG under
24+
* function JIT. The getter frame is pushed by the SIMPLE_GET fast path in the
25+
* shared FETCH_OBJ_R handler, but the JIT-compiled FUNC_ARG opcode had no
26+
* hook-enter guard, so the argument slot was read before the getter ran.
27+
*
28+
* The assertion is deterministic: the getter sets a static flag as a side
29+
* effect, so we check whether the getter actually ran when the property is
30+
* passed through FETCH_OBJ_FUNC_ARG. This avoids the previous data-dependent
31+
* failure mode (relying on file_get_contents() fataling on whatever garbage
32+
* the unguarded JIT read happened to land on), which made the regression
33+
* test flaky. */
34+
class Container {
35+
private static bool $getterRan = false;
36+
37+
public protected(set) HandlerInterface $handler;
38+
39+
public string $path {
40+
get => (self::$getterRan = true)
41+
? self::build($this->kind, $this->id)
42+
: self::build($this->kind, $this->id);
43+
}
44+
45+
protected mixed $prev = null;
46+
47+
public function __construct(
48+
public protected(set) string $kind,
49+
public protected(set) string $id,
50+
) {
51+
$this->handler = DefaultHandler::getInstance();
52+
}
53+
54+
public static function build(string $k, string $i): string {
55+
return "/nonexistent/gh22857_{$k}_{$i}.dat";
56+
}
57+
58+
public function step(): void {
59+
/* Unqualified namespaced-fallback call (INIT_NS_FCALL_BY_NAME) keeps
60+
* the FETCH_OBJ_FUNC_ARG opcode instead of letting the optimizer
61+
* rewrite it to FETCH_OBJ_R. @ preserves the opcode shape that
62+
* triggers the bug. */
63+
@file_get_contents($this->path);
64+
if (!self::$getterRan) {
65+
throw new \RuntimeException('getter did not run via FUNC_ARG');
66+
}
67+
}
68+
}
69+
70+
$c = new Container('alpha', 'beta');
71+
$c->step();
72+
$c->step();
73+
$c->step();
74+
75+
/* Sibling-slot variant: a preceding plain FETCH_OBJ_R primes the
76+
* SIMPLE_GET bit on the property cache slot; compact_literals shares the
77+
* slot between FETCH_OBJ_R and FETCH_OBJ_FUNC_ARG for the same property,
78+
* so the following FETCH_OBJ_FUNC_ARG consumes that bit and hits the
79+
* SIMPLE_GET fast path. Without the hook-enter guard it passes whatever
80+
* sits in an adjacent property slot instead of running the getter. The
81+
* flag is reset after the priming FETCH_OBJ_R so the assertion reflects
82+
* only whether the getter ran during the FETCH_OBJ_FUNC_ARG read. */
83+
class Container2 {
84+
private static bool $getterRan = false;
85+
86+
public protected(set) HandlerInterface $handler;
87+
88+
public string $path {
89+
get => (self::$getterRan = true)
90+
? self::build($this->kind, $this->id)
91+
: self::build($this->kind, $this->id);
92+
}
93+
94+
protected mixed $prev = null;
95+
96+
public function __construct(
97+
public protected(set) string $kind,
98+
public protected(set) string $id,
99+
) {
100+
$this->handler = DefaultHandler::getInstance();
101+
}
102+
103+
public static function build(string $k, string $i): string {
104+
return "/nonexistent/gh22857b_{$k}_{$i}.dat";
105+
}
106+
107+
public function step(): void {
108+
$this->prev = $this->path; // FETCH_OBJ_R primes SIMPLE_GET on shared slot
109+
self::$getterRan = false; // reset; flag now reflects only the FUNC_ARG read below
110+
@file_get_contents($this->path); // FETCH_OBJ_FUNC_ARG consumes the primed bit
111+
if (!self::$getterRan) {
112+
throw new \RuntimeException('sibling-slot variant: getter did not run via FUNC_ARG');
113+
}
114+
}
115+
}
116+
117+
$c2 = new Container2('alpha', 'beta');
118+
$c2->step();
119+
$c2->step();
120+
$c2->step();
121+
122+
echo "OK\n";
123+
?>
124+
--EXPECT--
125+
OK
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
--TEST--
2+
GH-22857 (reg-alloc): FETCH_OBJ_FUNC_ARG hook read must not lose register-held vars
3+
--ENV--
4+
A=1
5+
--INI--
6+
opcache.enable=1
7+
opcache.enable_cli=1
8+
opcache.jit_buffer_size=64M
9+
opcache.jit=1205
10+
opcache.jit_hot_func=1
11+
--FILE--
12+
<?php
13+
14+
class C {
15+
public $prop {
16+
get { return 1; }
17+
}
18+
}
19+
20+
function f(int $a0, $obj) {
21+
// $a lives only in a register: every use is in a supported opcode and in a
22+
// non-entry basic block, so the register allocator never spills it to the
23+
// VM stack. Exiting to the VM (the buggy hook-enter guard) would read a
24+
// stale stack slot for $a.
25+
$a = $a0;
26+
$b = $a + 2;
27+
$c = g($obj->prop, $a ? 1 : 2);
28+
return $b + $c;
29+
}
30+
31+
if (getenv('A')) {
32+
function g($a, $b) {
33+
var_dump($b);
34+
return $a;
35+
}
36+
}
37+
38+
$c = new C();
39+
var_dump(f(1, $c));
40+
var_dump(f(1, $c));
41+
42+
?>
43+
--EXPECT--
44+
int(1)
45+
int(4)
46+
int(1)
47+
int(4)

0 commit comments

Comments
 (0)