Attempt to fix GH-10008: Narrowing occurred during type inference of ZEND_ADD_ARRAY_ELEMENT #10294
Conversation
|
Although MAY_BE_ARRAY_PACKED is added here: php-src/Zend/Optimizer/zend_inference.c Line 1973 in 306a72a arr_type is 0 in this case. When assign_dim_array_result_type is called a second time, arr_type is 0x40804080 and MAY_BE_ARRAY_PACKED is not added.
The issue may be that assign_dim_array_result_type returns a two wide type with arr_type=0. I think that we are supposed to have assign_dim_array_result_type(x) >= assign_dim_array_result_type(y) for all arr_types x and y with x >= y. For reference, the issue has been introduced in 1ffbb73. |
|
Right... that makes sense
That sounds like the issue indeed and makes sense as an invariant, thanks for analyzing. |
…_ARRAY_ELEMENT This test triggers narrowing for two ops: first ZEND_ADD_ARRAY_ELEMENT, and then ZEND_ASSIGN. The type inference happens in the following order: 1) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080 (packed flag is set), arr_type=0 at this point because it hasn't been set by ZEND_INIT_ARRAY yet. 2) The ZEND_INIT_ARRAY infers type 0x40804080 3) The ZEND_ADD_ARRAY_ELEMENT infers type 0x40e04080, arr_type=0x40804080, which does not have the packed flag set while the existing result of ZEND_ADD_ARRAY_ELEMENT has the packed flag set. This seems to occur because of the phi node introduced by the while loop. If I remove the loop the problem goes away. As Arnaud noted, this seems to be caused by a too wide type inference for arr_type==0. We should keep the invariant that if x>=y then key_type(x) >= key_type(y). If we write the possible results down in a table we get: ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) ``` As we can see, `HASH_ONLY > 0` but `MAY_BE_ARRAY_NUMERIC_HASH < MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED`, which violates the invariant. Instead if we modify the zero case to have MAY_BE_ARRAY_NUMERIC_HASH instead, we get the following table which satisfies the invariant. ``` arr_type resulting key type --------------- -------------------------------------------------------------------------- HASH_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH PACKED_ONLY -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) HASH || PACKED -> MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED (== MAY_BE_ARRAY_KEY_LONG) 0 -> MAY_BE_ARRAY_NUMERIC_HASH ```
nielsdos
force-pushed
the
attempt-fix-10008
branch
from
de07248 to
8979d56
Compare
|
I looked at this again. CURRENTLYAs we can see, AFTER THIS PATCHInstead if we modify the zero case to have MAY_BE_ARRAY_NUMERIC_HASH instead we get the following table which satisfies the invariant. |
|
This has the unfortunate effect of always widening MAY_BE_ARRAY_PACKED to MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED when adding a numeric key to the array. I'm not sure this is a significant issue, but after looking more into it I believe that we can avoid this. We should have the same issue with this code: It turns out that this is prevented in php-src/Zend/Optimizer/zend_inference.c Lines 2304 to 2307 in 7200bc2
All oplines are skipped initially, so the problem does not happen. The oplines are visited again after In gh10008.phpt we don't skip |
|
Thanks for the review.
This is a pre-existing problem though, no?
By adding this in the else branch of if (arr_type == 0) {
break;
}However, I don't understand how this avoids the problem with packed that you mentioned. function test()
{
$bool = true;
for ($i = 0; $i < 10; $i++) {
if ($bool !== true) {
$array = [$bool, 123];
}
$bool = false;
}
}So I'm a bit confused honestly. |
|
Indeed, you are right. I was not expected that MAY_BE_ARRAY_PACKED was never returned on its own. The only references to MAY_BE_PACKED outside of zend_inference.c are in zend_jit_trace.c, and appear to be used in conditions to add jit guards (for arrays that are both PACKED and HASH), so the JIT may benefit from returning just MAY_BE_PACKED in the future, as this would remove some guards. Looks good to me then. Just a nit pick on the name |
|
Thank you Arnaud, I've fixed the name to |
|
I had to revert the fix. It passes CI tests in 8.1, but the JIT segfaults on 2 tests in 8.2 and higher. |
Attempt to fix GH-10008, see #10008 (comment)
This test triggers narrowing for two ops: first ZEND_ADD_ARRAY_ELEMENT, and then ZEND_ASSIGN.
The type inference happens in the following order:
This seems to occur because of the phi node introduced by the while loop. If I remove the loop the problem goes away.
As Arnaud noted, this seems to be caused by a too wide type inference for arr_type==0. We should keep the invariant that if x>=y then key_type(x) >= key_type(y).
If we write the possible results down in a table we get:
As we can see,
HASH_ONLY > 0butMAY_BE_ARRAY_NUMERIC_HASH < MAY_BE_ARRAY_NUMERIC_HASH | MAY_BE_ARRAY_PACKED, which violates the invariant.Instead if we modify the zero case to have MAY_BE_ARRAY_NUMERIC_HASH instead, we get the following table which satisfies the invariant.