Skip to content

Commit 80dc4c1

Browse files
iliaaldstogov
iliaal
and
dstogov
authored
Fix GH-20838: JIT compiler produces wrong arithmetic results (#21383)
Insert type guards (CHECK_OP1_TRACE_TYPE / CHECK_OP2_TRACE_TYPE) on the sensitive bailout paths in ADD/SUB/MUL JIT compilation: the MAY_BE_UNDEF and non-numeric operand breaks. Guards are only emitted when the traced operand type is IS_LONG or IS_DOUBLE, ensuring TSSA result type predictions stay valid for side traces without affecting the normal numeric fast path. Fixes GH-20838 Co-authored-by: Dmitry Stogov <dmitrystogov@gmail.com>
1 parent 1b61d55 commit 80dc4c1

File tree

3 files changed

+131
-0
lines changed

3 files changed

+131
-0
lines changed

‎ext/opcache/jit/zend_jit_trace.c‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4510,6 +4510,12 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
45104510
op2_info = OP2_INFO();
45114511
op2_addr = OP2_REG_ADDR();
45124512
if ((op1_info & MAY_BE_UNDEF) || (op2_info & MAY_BE_UNDEF)) {
4513+
if (op1_type == IS_LONG || op1_type == IS_DOUBLE) {
4514+
CHECK_OP1_TRACE_TYPE();
4515+
}
4516+
if (op2_type == IS_LONG || op2_type == IS_DOUBLE) {
4517+
CHECK_OP2_TRACE_TYPE();
4518+
}
45134519
break;
45144520
}
45154521
if (opline->opcode == ZEND_ADD &&
@@ -4518,6 +4524,12 @@ static const void *zend_jit_trace(zend_jit_trace_rec *trace_buffer, uint32_t par
45184524
/* pass */
45194525
} else if (!(op1_info & (MAY_BE_LONG|MAY_BE_DOUBLE)) ||
45204526
!(op2_info & (MAY_BE_LONG|MAY_BE_DOUBLE))) {
4527+
if (op1_type == IS_LONG || op1_type == IS_DOUBLE) {
4528+
CHECK_OP1_TRACE_TYPE();
4529+
}
4530+
if (op2_type == IS_LONG || op2_type == IS_DOUBLE) {
4531+
CHECK_OP2_TRACE_TYPE();
4532+
}
45214533
break;
45224534
}
45234535
if (orig_op1_type != IS_UNKNOWN

‎ext/opcache/tests/jit/gh20838.inc‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
<?php
2+
$result = json_decode('[{"ID":"49"},{"ID":"1080"},{"ID":"4415"},{"ID":"4763"},{"ID":"4926"},{"ID":"4933"},{"ID":"4935"},{"ID":"4938"},{"ID":"4939"},{"ID":"4947"},{"ID":"4952"},{"ID":"4953"},{"ID":"4962"},{"ID":"8558"},{"ID":"14888"},{"ID":"16879"},{"ID":"100003"},{"ID":"100007"},{"ID":"100013"},{"ID":"100017"},{"ID":"100019"},{"ID":"100027"},{"ID":"100031"},{"ID":"106427"},{"ID":"217955"},{"ID":"240000"},{"ID":"240010"},{"ID":"240021"},{"ID":"240079"},{"ID":"240210"},{"ID":"240227"},{"ID":"240249"},{"ID":"240273"},{"ID":"240333"},{"ID":"240335"},{"ID":"300024"},{"ID":"310001"},{"ID":"310025"},{"ID":"310034"},{"ID":"310042"},{"ID":"310111"},{"ID":"310191"},{"ID":"310219"},{"ID":"310236"},{"ID":"310322"},{"ID":"310356"},{"ID":"310360"},{"ID":"310394"},{"ID":"310405"},{"ID":"310411"}]', true);
3+
4+
$result1 = json_decode('[{"ID":"1080","cat":"6","vote":"2"},{"ID":"4415","cat":"1","vote":"5"},{"ID":"4415","cat":"5","vote":"-5"},{"ID":"4763","cat":"1","vote":"5"},{"ID":"4926","cat":"1","vote":"5"},{"ID":"4933","cat":"1","vote":"4"},{"ID":"4935","cat":"1","vote":"5"},{"ID":"4938","cat":"1","vote":"5"},{"ID":"4939","cat":"1","vote":"5"},{"ID":"4947","cat":"1","vote":"4"},{"ID":"4952","cat":"1","vote":"3"},{"ID":"4953","cat":"1","vote":"5"},{"ID":"4962","cat":"1","vote":"4"},{"ID":"8558","cat":"1","vote":"3"},{"ID":"14888","cat":"4","vote":"5"},{"ID":"16879","cat":"4","vote":"-2"},{"ID":"16879","cat":"6","vote":"3"},{"ID":"100003","cat":"1","vote":"0"},{"ID":"100007","cat":"1","vote":"4"},{"ID":"100013","cat":"1","vote":"0"},{"ID":"100017","cat":"6","vote":"4"},{"ID":"100017","cat":"4","vote":"-4"},{"ID":"100019","cat":"1","vote":"1"},{"ID":"100027","cat":"1","vote":"3"},{"ID":"100031","cat":"1","vote":"2"},{"ID":"106427","cat":"5","vote":"-5"},{"ID":"217955","cat":"4","vote":"-1"},{"ID":"217955","cat":"4","vote":"-1"},{"ID":"240000","cat":"1","vote":"2"},{"ID":"240010","cat":"1","vote":"4"},{"ID":"240021","cat":"1","vote":"3"},{"ID":"240079","cat":"1","vote":"4"},{"ID":"240079","cat":"7","vote":"4"},{"ID":"240079","cat":"6","vote":"2"},{"ID":"240210","cat":"1","vote":"3"},{"ID":"240227","cat":"1","vote":"3"},{"ID":"240249","cat":"1","vote":"5"},{"ID":"240273","cat":"1","vote":"5"},{"ID":"240333","cat":"1","vote":"2"},{"ID":"240335","cat":"1","vote":"4"},{"ID":"300024","cat":"3","vote":"4"},{"ID":"300024","cat":"1","vote":"5"},{"ID":"310001","cat":"1","vote":"3"},{"ID":"310025","cat":"1","vote":"3"},{"ID":"310034","cat":"1","vote":"4"},{"ID":"310042","cat":"1","vote":"2"},{"ID":"310111","cat":"1","vote":"5"},{"ID":"310191","cat":"1","vote":"3"},{"ID":"310219","cat":"1","vote":"5"},{"ID":"310236","cat":"1","vote":"1"},{"ID":"310322","cat":"1","vote":"3"},{"ID":"310356","cat":"1","vote":"2"},{"ID":"310360","cat":"1","vote":"3"},{"ID":"310360","cat":"4","vote":"-5"},{"ID":"310394","cat":"1","vote":"1"},{"ID":"310394","cat":"3","vote":"2"},{"ID":"310405","cat":"1","vote":"5"},{"ID":"310411","cat":"1","vote":"4"}]', true);
5+
6+
foreach ($result as $arr) {
7+
$v = [];
8+
foreach ($result1 as $arr1) {
9+
if ($arr1['ID'] == $arr['ID']) $v[] = $arr1;
10+
}
11+
TLVotesStatUpdateBAD($arr['ID'], $v);
12+
}
13+
14+
function TLVotesStatUpdateBAD(int $id, ?array $v = null): float
15+
{
16+
static $k;
17+
if (!isset($k)) $k = json_decode('{"4":{"tp":"20","st":"sum"},"3":{"tp":"30","st":"caa"},"2":{"tp":"60","st":"sum"},"1":{"tp":"50","st":"unique"},"5":{"tp":"50","st":"caa"},"6":{"tp":"20","st":"caa"},"7":{"tp":"60","st":"caa"},"8":{"tp":"60","st":"caa"},"255":{"tp":"0","st":"correction"}}', true);
18+
19+
$pav = 50;
20+
$s = 0.0;
21+
$corrections = 0;
22+
$av = [];
23+
$avc = [];
24+
25+
foreach ($v as $arr) {
26+
if ($k[$arr['cat']]['st'] == 'unique') $av[$arr['cat']] = $arr['vote'];
27+
elseif ($k[$arr['cat']]['st'] == 'correction') {
28+
$av[$arr['cat']] = $arr['vote'];
29+
$corrections++;
30+
} else {
31+
if ($arr['vote'] < 0) $arr['vote'] = $arr['vote'] / 2;
32+
$av[$arr['cat']] ??= 0;
33+
$avc[$arr['cat']] ??= 0;
34+
$av[$arr['cat']] += $arr['vote'];
35+
$avc[$arr['cat']]++;
36+
}
37+
}
38+
39+
if (($c = count($av))) {
40+
$c -= $corrections;
41+
foreach ($av as $key => $value) {
42+
if ($k[$key]['st'] == 'correction') $s += $value;
43+
else {
44+
if ($k[$key]['st'] == 'caa') $value = $value / $avc[$key];
45+
$s += $value / 5 * $k[$key]['tp'] * (1 + 50 / $k[$key]['tp'] * $pav / (100 * $c));
46+
}
47+
}
48+
}
49+
50+
echo("$id $s\n");
51+
52+
return $s;
53+
}

‎ext/opcache/tests/jit/gh20838.phpt‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
--TEST--
2+
GH-20838 (JIT compiler produces wrong arithmetic results)
3+
--INI--
4+
opcache.enable=1
5+
opcache.enable_cli=1
6+
opcache.jit=tracing
7+
opcache.jit_buffer_size=64M
8+
opcache.jit_hot_loop=61
9+
opcache.jit_hot_func=127
10+
opcache.jit_hot_return=8
11+
opcache.jit_hot_side_exit=8
12+
--EXTENSIONS--
13+
opcache
14+
--FILE_EXTERNAL--
15+
gh20838.inc
16+
--EXPECT--
17+
49 0
18+
1080 18
19+
4415 31.25
20+
4763 75
21+
4926 75
22+
4933 60
23+
4935 75
24+
4938 75
25+
4939 75
26+
4947 60
27+
4952 45
28+
4953 75
29+
4962 60
30+
8558 45
31+
14888 45
32+
16879 13
33+
100003 0
34+
100007 60
35+
100013 0
36+
100017 13
37+
100019 15
38+
100027 45
39+
100031 30
40+
106427 -37.5
41+
217955 -9
42+
240000 30
43+
240010 60
44+
240021 45
45+
240079 112.66666666667
46+
240210 45
47+
240227 45
48+
240249 75
49+
240273 75
50+
240333 30
51+
240335 60
52+
300024 96.5
53+
310001 45
54+
310025 45
55+
310034 60
56+
310042 30
57+
310111 75
58+
310191 45
59+
310219 75
60+
310236 15
61+
310322 45
62+
310356 30
63+
310360 21.25
64+
310394 29.5
65+
310405 75
66+
310411 60

0 commit comments

Comments
 (0)