Skip to content

Commit debf3a8

Browse files
arnaud-lbarnaud-lb
committed
Fix array_map optimization with non-literal function or non-literal args
Non-literal expressions must be evaluated once and memoized to maintain semantics.
1 parent b6bd738 commit debf3a8

6 files changed

Lines changed: 657 additions & 23 deletions

‎Zend/zend_compile.c‎

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5301,11 +5301,64 @@ static zend_result zend_compile_func_array_map(znode *result, zend_ast_list *arg
53015301
return FAILURE;
53025302
}
53035303

5304+
/* Bail out if callback is not an FCC */
53045305
zend_ast *callback = args->child[0];
53055306
if (callback->kind != ZEND_AST_CALL && callback->kind != ZEND_AST_STATIC_CALL) {
53065307
return FAILURE;
53075308
}
53085309

5310+
zend_ast *args_ast = zend_ast_call_get_args(callback);
5311+
if (args_ast->kind != ZEND_AST_CALLABLE_CONVERT) {
5312+
return FAILURE;
5313+
}
5314+
5315+
/* PFAs with non-literal pre-bound arguments are not optimizable because we
5316+
* can't memoize arguments without breaking pass-by-reference.
5317+
* TODO: Support PFAs when the function is known to not receive by ref. */
5318+
zend_ast_fcc *fcc = (zend_ast_fcc*)args_ast;
5319+
zend_ast_list *fcc_args = zend_ast_get_list(fcc->args);
5320+
for (uint32_t i = 0; i < fcc_args->children; i++) {
5321+
zend_ast *arg = fcc_args->child[i];
5322+
if (arg->kind == ZEND_AST_NAMED_ARG) {
5323+
arg = arg->child[1];
5324+
}
5325+
5326+
if (arg->kind == ZEND_AST_PLACEHOLDER_ARG) {
5327+
continue;
5328+
}
5329+
5330+
if (arg->kind != ZEND_AST_ZVAL) {
5331+
return FAILURE;
5332+
}
5333+
}
5334+
5335+
/* Evaluate class name */
5336+
znode class_node;
5337+
if (callback->kind == ZEND_AST_STATIC_CALL) {
5338+
znode result;
5339+
zend_compile_expr(&result, callback->child[0]);
5340+
if (result.op_type == IS_CONST) {
5341+
class_node = result;
5342+
} else {
5343+
class_node = result;
5344+
zend_emit_op_tmp(&class_node, ZEND_QM_ASSIGN, &result, NULL);
5345+
}
5346+
} else {
5347+
class_node.op_type = IS_UNUSED;
5348+
}
5349+
5350+
/* Evaluate function name */
5351+
znode func_node;
5352+
{
5353+
znode result;
5354+
zend_compile_expr(&result, callback->child[callback->kind == ZEND_AST_CALL ? 0 : 1]);
5355+
if (result.op_type == IS_CONST) {
5356+
func_node = result;
5357+
} else {
5358+
zend_emit_op_tmp(&func_node, ZEND_QM_ASSIGN, &result, NULL);
5359+
}
5360+
}
5361+
53095362
znode value;
53105363
value.op_type = IS_TMP_VAR;
53115364
value.u.op.var = get_temporary_variable();
@@ -5314,6 +5367,12 @@ static zend_result zend_compile_func_array_map(znode *result, zend_ast_list *arg
53145367
zend_ast_create_znode(&value));
53155368
if (!call_args) {
53165369
CG(active_op_array)->T--;
5370+
if (func_node.op_type == IS_CONST) {
5371+
zval_ptr_dtor_nogc(&func_node.u.constant);
5372+
}
5373+
if (class_node.op_type == IS_CONST) {
5374+
zval_ptr_dtor_nogc(&class_node.u.constant);
5375+
}
53175376
/* The callback is not a FCC/PFA, or is not optimizable */
53185377
return FAILURE;
53195378
}
@@ -5353,14 +5412,35 @@ static zend_result zend_compile_func_array_map(znode *result, zend_ast_list *arg
53535412

53545413
/* loop body */
53555414
znode call_result;
5415+
zend_ast *func_ast;
5416+
if (func_node.op_type == IS_CONST) {
5417+
func_ast = zend_ast_create_znode(&func_node);
5418+
} else {
5419+
znode copy_node;
5420+
zend_emit_op_tmp(&copy_node, ZEND_COPY_TMP, &func_node, NULL);
5421+
func_ast = zend_ast_create_znode(&copy_node);
5422+
}
53565423
switch (callback->kind) {
5357-
case ZEND_AST_CALL:
5358-
zend_compile_expr(&call_result, zend_ast_create(ZEND_AST_CALL, callback->child[0], call_args));
5424+
case ZEND_AST_CALL: {
5425+
zend_compile_expr(&call_result, zend_ast_create(ZEND_AST_CALL, func_ast, call_args));
53595426
break;
5360-
case ZEND_AST_STATIC_CALL:
5361-
zend_compile_expr(&call_result, zend_ast_create(ZEND_AST_STATIC_CALL, callback->child[0], callback->child[1], call_args));
5427+
}
5428+
case ZEND_AST_STATIC_CALL: {
5429+
zend_ast *class_ast;
5430+
if (class_node.op_type == IS_CONST) {
5431+
class_ast = zend_ast_create_znode(&class_node);
5432+
} else {
5433+
znode copy_node;
5434+
zend_emit_op_tmp(&copy_node, ZEND_COPY_TMP, &class_node, NULL);
5435+
class_ast = zend_ast_create_znode(&copy_node);
5436+
}
5437+
5438+
zend_compile_expr(&call_result, zend_ast_create(ZEND_AST_STATIC_CALL, class_ast, func_ast, call_args));
5439+
zend_ast_destroy(class_ast);
53625440
break;
5441+
}
53635442
}
5443+
zend_ast_destroy(func_ast);
53645444
opline = zend_emit_op(NULL, ZEND_ADD_ARRAY_ELEMENT, &call_result, &key);
53655445
SET_NODE(opline->result, result);
53665446
/* end loop body */
@@ -5375,6 +5455,12 @@ static zend_result zend_compile_func_array_map(znode *result, zend_ast_list *arg
53755455

53765456
zend_end_loop(opnum_fetch, &reset_node);
53775457
zend_emit_op(NULL, ZEND_FE_FREE, &reset_node, NULL);
5458+
if (func_node.op_type != IS_CONST) {
5459+
zend_emit_op(NULL, ZEND_FREE, &func_node, NULL);
5460+
}
5461+
if (class_node.op_type != IS_UNUSED && class_node.op_type != IS_CONST) {
5462+
zend_emit_op(NULL, ZEND_FREE, &class_node, NULL);
5463+
}
53785464

53795465
return SUCCESS;
53805466
}

‎ext/opcache/tests/array_map_foreach_optimization_006.phpt‎

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -32,26 +32,30 @@ $_main:
3232
0003 T3 = DO_ICALL
3333
0004 ASSIGN CV0($array) T3
3434
0005 ASSIGN CV1($plus1) string("plus1")
35-
0006 TYPE_ASSERT 131079 string("array_map") CV0($array)
36-
0007 T3 = INIT_ARRAY 0 (packed) NEXT
37-
0008 V4 = FE_RESET_R CV0($array) 0015
38-
0009 T6 = FE_FETCH_R V4 T5 0015
39-
0010 INIT_DYNAMIC_CALL 1 CV1($plus1)
40-
0011 SEND_VAL_EX T5 1
41-
0012 T5 = DO_FCALL
42-
0013 T3 = ADD_ARRAY_ELEMENT T5 T6
43-
0014 JMP 0009
44-
0015 FE_FREE V4
45-
0016 ASSIGN CV2($foo) T3
46-
0017 INIT_FCALL 1 %d string("var_dump")
47-
0018 SEND_VAR CV2($foo) 1
48-
0019 DO_ICALL
49-
0020 RETURN int(1)
35+
0006 T4 = QM_ASSIGN CV1($plus1)
36+
0007 TYPE_ASSERT 131079 string("array_map") CV0($array)
37+
0008 T3 = INIT_ARRAY 0 (packed) NEXT
38+
0009 V5 = FE_RESET_R CV0($array) 0017
39+
0010 T7 = FE_FETCH_R V5 T6 0017
40+
0011 T8 = COPY_TMP T4
41+
0012 INIT_DYNAMIC_CALL 1 T8
42+
0013 SEND_VAL_EX T6 1
43+
0014 T6 = DO_FCALL
44+
0015 T3 = ADD_ARRAY_ELEMENT T6 T7
45+
0016 JMP 0010
46+
0017 FE_FREE V5
47+
0018 FREE T4
48+
0019 ASSIGN CV2($foo) T3
49+
0020 INIT_FCALL 1 %d string("var_dump")
50+
0021 SEND_VAR CV2($foo) 1
51+
0022 DO_ICALL
52+
0023 RETURN int(1)
5053
LIVE RANGES:
51-
3: 0008 - 0016 (tmp/var)
52-
4: 0009 - 0015 (loop)
53-
5: 0010 - 0011 (tmp/var)
54-
6: 0010 - 0013 (tmp/var)
54+
4: 0007 - 0018 (tmp/var)
55+
3: 0009 - 0019 (tmp/var)
56+
5: 0010 - 0017 (loop)
57+
6: 0011 - 0013 (tmp/var)
58+
7: 0011 - 0015 (tmp/var)
5559

5660
plus1:
5761
; (lines=3, args=1, vars=1, tmps=%d)

‎ext/opcache/tests/array_map_foreach_optimization_009.phpt‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
--TEST--
2+
array_map(): foreach optimization - dynamic-call drift bug
3+
--CREDITS--
4+
Ryan @ Calif.io
5+
--EXTENSIONS--
6+
opcache
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
--FILE--
11+
<?php
12+
13+
function cand_86013_trusted(string $value): string
14+
{
15+
return 'trusted:' . $value;
16+
}
17+
18+
function cand_86013_unexpected(string $value): string
19+
{
20+
return 'unexpected:' . $value;
21+
}
22+
23+
function cand_86013_input_changes_target(): array
24+
{
25+
global $callback;
26+
global $obj;
27+
$callback = 'cand_86013_unexpected';
28+
$obj = new Unexpected;
29+
return ['payload'];
30+
}
31+
32+
class Trusted {
33+
static function f($value) {
34+
return 'trusted:' . $value;
35+
}
36+
}
37+
38+
class Unexpected {
39+
static function f($value) {
40+
return 'unexpected:' . $value;
41+
}
42+
}
43+
44+
$callback = 'cand_86013_trusted';
45+
echo "direct array_map\n";
46+
var_dump(array_map($callback(...), cand_86013_input_changes_target()));
47+
48+
$callback = 'cand_86013_trusted';
49+
$array_map = 'array_map';
50+
echo "dynamic-call control\n";
51+
var_dump($array_map($callback(...), cand_86013_input_changes_target()));
52+
53+
$missing = 'cand_86013_missing';
54+
echo "empty direct\n";
55+
try {
56+
var_dump(array_map($missing(...), []));
57+
} catch (Throwable $e) {
58+
echo get_class($e), ': ', $e->getMessage(), "\n";
59+
}
60+
61+
echo "empty dynamic-call control\n";
62+
try {
63+
var_dump($array_map($missing(...), []));
64+
} catch (Throwable $e) {
65+
echo get_class($e), ': ', $e->getMessage(), "\n";
66+
}
67+
68+
$obj = new Trusted;
69+
echo "direct array_map static call\n";
70+
var_dump(array_map($obj::f(...), cand_86013_input_changes_target()));
71+
72+
?>
73+
--EXPECT--
74+
direct array_map
75+
array(1) {
76+
[0]=>
77+
string(15) "trusted:payload"
78+
}
79+
dynamic-call control
80+
array(1) {
81+
[0]=>
82+
string(15) "trusted:payload"
83+
}
84+
empty direct
85+
array(0) {
86+
}
87+
empty dynamic-call control
88+
Error: Call to undefined function cand_86013_missing()
89+
direct array_map static call
90+
array(1) {
91+
[0]=>
92+
string(15) "trusted:payload"
93+
}

‎ext/opcache/tests/array_map_foreach_optimization_010.phpt‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
--TEST--
2+
array_map(): foreach optimization - pfa pre-bound arg reexecution bug
3+
--CREDITS--
4+
Ryan @ Calif.io
5+
--EXTENSIONS--
6+
opcache
7+
--INI--
8+
opcache.enable=1
9+
opcache.enable_cli=1
10+
--FILE--
11+
<?php
12+
13+
function cand_86013_bound_value(): string
14+
{
15+
global $bound_calls;
16+
echo 'BOUND:', ++$bound_calls, "\n";
17+
return 'b';
18+
}
19+
20+
function cand_86013_input(): array
21+
{
22+
echo "INPUT\n";
23+
return ['a', 'a', 'a'];
24+
}
25+
26+
$bound_calls = 0;
27+
echo "direct array_map\n";
28+
$direct = array_map(
29+
str_replace('a', cand_86013_bound_value(), ?),
30+
cand_86013_input(),
31+
);
32+
var_dump($direct, $bound_calls);
33+
34+
$bound_calls = 0;
35+
$array_map = 'array_map';
36+
echo "dynamic-call control\n";
37+
$control = $array_map(
38+
str_replace('a', cand_86013_bound_value(), ?),
39+
cand_86013_input(),
40+
);
41+
var_dump($control, $bound_calls);
42+
43+
?>
44+
--EXPECT--
45+
direct array_map
46+
BOUND:1
47+
INPUT
48+
array(3) {
49+
[0]=>
50+
string(1) "b"
51+
[1]=>
52+
string(1) "b"
53+
[2]=>
54+
string(1) "b"
55+
}
56+
int(1)
57+
dynamic-call control
58+
BOUND:1
59+
INPUT
60+
array(3) {
61+
[0]=>
62+
string(1) "b"
63+
[1]=>
64+
string(1) "b"
65+
[2]=>
66+
string(1) "b"
67+
}
68+
int(1)

0 commit comments

Comments
 (0)