Bug #76466 [Ana]: Loop variable confusion

Edit report at https://bugs.php.net/bug.php?id=76466&edit=1 ID: 76466 Updated by: nikic@php.net Reported by: cmb@php.net Summary: Loop variable confusion Status: Analyzed Type: Bug Package: opcache Operating System: Linux PHP Version: 7.3.0alpha1 Assigned To: laruence Block user comment: N Private report: N New Comment: @dmitry: It should not be necessary to manually BOT out the chain. If you only set the current result to BOT, the value will be propagated. Of course returning BOT here will prevent us from catching certain patterns, though probably they aren't particularly important to us. Previous Comments: ------------------------------------------------------------------------ [2018-06-15 09:59:31] dmitry@php.net @laruence @nikic I propose another approach - make BOT all the chain of ADD_ARRAY_ELEMENT opcodes, when some variable changes. Please review https://gist.github.com/dstogov/142223b0c42c49a523f72a814db8b4f0 ------------------------------------------------------------------------ [2018-06-14 14:47:08] laruence@php.net @nikic probably you saw the old patch？ the result parts are already changed :) about the array size limit, yeah, it's good idea... as after this change the array numbers will increased visibly. I've asked dmitry to also have a look.. let's see what's his opinion . ------------------------------------------------------------------------ [2018-06-14 09:46:55] nikic@php.net @laruence: Yes, this is clearly wrong. Without the partial array propagation this would still be fine if we made sure to propagate a BOT op1/op2 before the check (which we didn't do either...), as we know that these are the only possible lowerings and both result in a BOT. However, with partial arrays this is no longer possible, so I'd say we should remove this (you'll also have to modify the if (result) code below) and instead add a check for the array size and return BOT if the array is larger than say 16 elements, to avoid quadratic blowup. ------------------------------------------------------------------------ [2018-06-14 09:26:54] laruence@php.net the problem is that we may visit one instruction multiply times, depends on new infos comes... let me try to explain: let say we have two ops #1 INIT_ARRAY #2 ADD_ARRAY_ELEMENT when we first visit INIT_ARRAY, we have op1 is an const value, then we set INIT_ARRAY’s result as a const array. then we visist #2, we get the result value from #1, and set the #1 result to NULL, then make #2 result a const array. but later, INIT_ARRAY op1’s value become BOT… and #1 is added into work instr again… but this time, we visist #1, we get INIT_ARRAY->Result.def is IS_NULL, then we simply return, then we leave #2’s result as an incorrect value type…. (it should be BOT, but it’s array) fix could be something like https://gist.github.com/laruence/4068c564e38e8e0f84148cc1b3f977cf ------------------------------------------------------------------------ [2018-06-13 13:17:41] cmb@php.net Looks like the SCCP is too aggressive as of commit ce3dbb5[1]. For what it's worth, removing the if statement from the test script gives expected results. [1] <https://github.com/php/php-src/commit/ce3dbb53a7dcef314bba1d355f049ddb2252fad1> ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=76466 -- Edit this bug report at https://bugs.php.net/bug.php?id=76466&edit=1