# Undefined variables undefined order - Thread: https://externals.io/message/86158 - Original archive: https://news-web.php.net/php.internals/86158 - Started by Sara Golemon on 2015-05-11T22:38:17+00:00 - Messages: 17 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #86158 — Sara Golemon — 2015-05-11T22:38:17+00:00 URL: https://externals.io/message/86158#86158 http://3v4l.org/uVNIS Prior to PHP 5.1 (and the introduction of compiled variables), the following code would output warnings for the undefined variables in the order they were used: echo $x . $y . $z; However, with the introduction of CVs, we wind up getting the warning for $y, then $x, and finally $z. ((Insert "middle-out" reference from HBO's Silicon Valley, here)) The reason for this, is that ZEND_CONCAT (and indeed, most binary operations) is implemented as a single inline function call: concat_function(EX_VAR(opline->result.var), GET_OP1_ZVAL_PTR(BP_VAR_R), GET_OP2_ZVAL_PTR(BP_VAR_R)); When gcc looks at this function call, it treats the execution order for the arguments as undefined. In practice (at least on 3v4l.org) it ends up evaluating the third argument (OP2) first, then the second argument (OP1), and finally the first argument (result.var). This is perfectly legal as far as GCC is concerned since the result of one function call argument shouldn't have side effects relative to another argument. I'd like to propose making the order of evaluation defined by splitting this into separate statements: zval *op1 = GET_OP1_ZVAL_PTR(BP_VAR_R); zval *op2 = GET_OP2_ZVAL_PTR(BP_VAR_R); concat_function(EX_VAR(opline->result.var), op1, op2); The optimizer should do a proper job of removing the intermediate assignment while keeping the defined resolution order intact. God help us if anyone is actually /depending/ on this behavior... -Sara --- ## #86159 — Stanislav Malyshev — 2015-05-11T23:26:28+00:00 URL: https://externals.io/message/86158#86159 · In reply to: #86158 Hi! > argument. I'd like to propose making the order of evaluation defined > by splitting this into separate statements: What is the purpose of this? I.e. why is it important that these notices would be produced in certain order? -- Stas Malyshev smalyshev@gmail.com --- ## #86160 — Matt Wilmas — 2015-05-12T00:12:12+00:00 URL: https://externals.io/message/86158#86160 · In reply to: #86159 Hi! ----- Original Message ----- From: "Stanislav Malyshev" Sent: Monday, May 11, 2015 > Hi! > > > […] > > What is the purpose of this? I.e. why is it important that these notices > would be produced in certain order? Logic, consistency, "polish," etc. :-) I never noticed this, but seems like a good change to make. I guess GCC, and other compilers, are just executing the arguments in the order they're push'ed (x86), which is reasonable and logical... > Stas Malyshev > smalyshev@gmail.com - Matt --- ## #86163 — Sara Golemon — 2015-05-12T04:05:59+00:00 URL: https://externals.io/message/86158#86163 · In reply to: #86160 > ----- Original Message ----- > From: "Stanislav Malyshev" > Sent: Monday, May 11, 2015 > > > […] > > Logic, consistency, "polish," etc. :-) > Exactly this. :) It's just my CDO kicking in. It's not necessary for the language to function. Heck, HHVM already matches PHP 5.1-7.0's out-of-order behavior, so fixing PHP only creates double work for me. I just look at these error messages and twitch uncomfortably. -Sara --- ## #86165 — Andi Gutmans — 2015-05-12T05:44:43+00:00 URL: https://externals.io/message/86158#86165 · In reply to: #86163 > > > > […] > Exactly this. :) > > It's just my CDO kicking in. It's not necessary for the language to > function. Heck, HHVM already matches PHP 5.1-7.0's out-of-order > behavior, so fixing PHP only creates double work for me. I just look > at these error messages and twitch uncomfortably. I don't think it is worth the change. I actually prefer to leave code path optimization up to gcc here vs. trying to tell it what to do. Andi > > -Sara --- ## #86169 — Sara Golemon — 2015-05-12T15:59:43+00:00 URL: https://externals.io/message/86158#86169 · In reply to: #86165 > I don't think it is worth the change. I actually prefer to leave code path > optimization up to gcc here vs. trying to tell it what to do. > This isn't about optimizing code paths, it's about changing undefined behavior into defined behavior. Either way the compiler is left to optimize and organize code according to its preferences. But whatever, this is so not something worth arguing over. If you like PHP being clowny in this minor edge case, I'm not going to get in your way. Cheers, -Sara --- ## #86521 — Matt Wilmas — 2015-06-09T13:05:30+00:00 URL: https://externals.io/message/86158#86521 · In reply to: #86163 Hi all, ----- Original Message ----- From: "Sara Golemon" Sent: Monday, May 11, 2015 > On Mon, May 11, 2015 at 5:12 PM, Matt Wilmas > wrote: > > […] > Exactly this. :) > > It's just my CDO kicking in. It's not necessary for the language to > function. Heck, HHVM already matches PHP 5.1-7.0's out-of-order > behavior, so fixing PHP only creates double work for me. I just look > at these error messages and twitch uncomfortably. Hah, looks like this just changed last week after barely 3 weeks. :-P I didn't verify, just noticed the code change: http://git.php.net/?p=php-src.git;a=commitdiff;h=c09698753e7d1d95299dca54c8ca888c885fd45b Andi hasn't objected, yet. :-O And ironically it was part of an optimization... Now CONCAT is consistent, but not others. Which places need changing anyway? Just binary ops in VM? A couple/few extra instructions are unavoidable, I guess on any architecture, for the intermediate save of op1, but can anyone show that it makes ANY measurable difference (other than instruction count)? For ADD, MUL, IS_[NOT_]IDENTICAL, IS_[NOT_]EQUAL, BW_*, and BOOL_XOR, ops can be passed as ... result, op2, op1); with no ill effects (I think) and still have the same (fewest) instructions as now! That is, if anyone thinks, or can show, that it matters. :-) Whoops, didn't check first -- I see ADD, MUL, and IS_[NOT_]EQUAL (among others) are already consistent/fixed (but extra instructions :-D). > -Sara - Matt --- ## #86527 — Dmitry Stogov — 2015-06-09T16:29:27+00:00 URL: https://externals.io/message/86158#86527 · In reply to: #86521 I don't see a big problem changing this for few opcodes in PHP-7 VM, and make behavior "more defined". On the other hand I don't think we shouldn't "define" the order of operand evaluation. This may prevent us performing optimisations in the future. Fixing this in PHP-5.* doesn't cost the effort from my point of view. Thanks. Dmitry. > Hi all, > > ----- Original Message ----- > From: "Sara Golemon" > Sent: Monday, May 11, 2015 > > On Mon, May 11, 2015 at 5:12 PM, Matt Wilmas > > […] > > Hah, looks like this just changed last week after barely 3 weeks. :-P I > didn't verify, just noticed the code change: > http://git.php.net/?p=php-src.git;a=commitdiff;h=c09698753e7d1d95299dca54c8ca888c885fd45b > > Andi hasn't objected, yet. :-O And ironically it was part of an > optimization... > > Now CONCAT is consistent, but not others. Which places need changing > anyway? Just binary ops in VM? > > A couple/few extra instructions are unavoidable, I guess on any > architecture, for the intermediate save of op1, but can anyone show that it > makes ANY measurable difference (other than instruction count)? > > For ADD, MUL, IS_[NOT_]IDENTICAL, IS_[NOT_]EQUAL, BW_*, and BOOL_XOR, ops > can be passed as > > ... result, op2, op1); > > with no ill effects (I think) and still have the same (fewest) > instructions as now! That is, if anyone thinks, or can show, that it > matters. :-) > > Whoops, didn't check first -- I see ADD, MUL, and IS_[NOT_]EQUAL (among > others) are already consistent/fixed (but extra instructions :-D). > > -Sara > > […] > > - Matt --- ## #86552 — Sara Golemon — 2015-06-10T14:42:30+00:00 URL: https://externals.io/message/86158#86552 · In reply to: #86521 > Hah, looks like this just changed last week after barely 3 weeks. :-P I > didn't verify, just noticed the code change: > http://git.php.net/?p=php-src.git;a=commitdiff;h=c09698753e7d1d95299dca54c8ca888c885fd45b > Dmitry, what's the reasoning behind this diff in the first place? Doesn't the compiler fold ( . ) already anyhow? How would we wind up with CONCAT_CONST_CONST at runtime? > Now CONCAT is consistent, but not others. Which places need changing > anyway? Just binary ops in VM? > > A couple/few extra instructions are unavoidable, I guess on any > architecture, for the intermediate save of op1, but can anyone show that it > makes ANY measurable difference (other than instruction count)? > I'm 90% certain that fixing the ordering (by saving to a temp var) doesn't *actually* result in extra instructions. GCC's optimizer is pretty clever. It'll notice you're only writing to the temp var once, and reading from it once (immediately), so it'll end up organizing the instructions in a way which satisfies the explicit order, but as an inline call. -Sara --- ## #86554 — Sara Golemon — 2015-06-10T14:50:26+00:00 URL: https://externals.io/message/86158#86554 · In reply to: #86552 > Dmitry, what's the reasoning behind this diff in the first place? > Doesn't the compiler fold ( . ) already > anyhow? How would we wind up with CONCAT_CONST_CONST at runtime? > Derp. Looked again, and it's (const *OR* string) && (const *OR* string). I read those as ANDs initially. But now I'm confused again, because that makes it look like we can reach this with an expression like "1 . 2" 1 and 2 are consts, but not strings, so then we get to: zend_string *op1_str = Z_STR_P(op1); Which will lead to op1_str pointing at 0x00000001 and a segfault. What am I missing here? -Sara --- ## #86555 — Sara Golemon — 2015-06-10T15:00:28+00:00 URL: https://externals.io/message/86158#86555 · In reply to: #86554 > > > […] > Derp. Looked again, and it's (const *OR* string) && (const *OR* > string). I read those as ANDs initially. > > But now I'm confused again, because that makes it look like we can > reach this with an expression like "1 . 2" 1 and 2 are consts, but not > strings, so then we get to: > > zend_string *op1_str = Z_STR_P(op1); > > Which will lead to op1_str pointing at 0x00000001 and a segfault. > > What am I missing here? > Okay, answered my own question (I need to be gentler with the [Send] button). Zend/zend_compile.c sets up an assumption: if (left_node.op_type == IS_CONST) { convert_to_string(&left_node.u.constant); } if (right_node.op_type == IS_CONST) { convert_to_string(&right_node.u.constant); } So any const nodes passed to CONCAT will always be strings (having been preconverted), and the if check using an OR make sense, because if it's CONST, then we KNOW it's a string, and there's no point testing for it. We won't ever reach CONCAT_CONST_CONST (since that would be folded in the compiler), but we might reach CONCAT_CONST_CV or CONCAT_TMPVAR_CONST or some other combination thereof for which this case is set to handle. TL;DR - Never mind me. I just didn't think it all the way through. -Sara P.S. - An assert(Z_TYPE_P(op1) == IS_STRING); and assert(Z_TYPE_P(op2) == IS_STRING); does seem reasonable though... Maybe even with a comment referencing that we can make that assumption because the compiler fixes up the types. --- ## #86565 — Matt Wilmas — 2015-06-10T15:44:01+00:00 URL: https://externals.io/message/86158#86565 · In reply to: #86552 Hi Sara, ----- Original Message ----- From: "Sara Golemon" Sent: Wednesday, June 10, 2015 > On Tue, Jun 9, 2015 at 6:05 AM, Matt Wilmas > wrote: > > […] > Dmitry, what's the reasoning behind this diff in the first place? > Doesn't the compiler fold ( . ) already > anyhow? How would we wind up with CONCAT_CONST_CONST at runtime? > > > […] > I'm 90% certain that fixing the ordering (by saving to a temp var) > doesn't *actually* result in extra instructions. GCC's optimizer is > pretty clever. It'll notice you're only writing to the temp var once, > and reading from it once (immediately), so it'll end up organizing the > instructions in a way which satisfies the explicit order, but as an > inline call. I don't know... Yeah, if op1 is coming from a "plain" variable, either something simple (like CONST?) or after function inlining. e.g. Like "directly" accessible, then it should be able to be done the same. (I don't know a lot about any of this though.) I was thinking of when some sort of function call is involved (as far as unavoidable extra instructions). If op1 is fetched after op2, it's optimized for passing to concat_function(), etc. Doing stuff in reverse order will need fewer instructions than "human order," at least when function calls are needed for "stuff." Hmm, maybe that's only for x86 with push'ed args, where each thing can keep push'ing whatever it needs (eax) for the final (outer) function... x86_64 is I guess still going to need to store stuff "out of the way" for the next (inner) call, before moving it back to the necessary registers for the final function! If that's true, maybe x86_64 doesn't need more instructions. I think I've confused myself more trying to picture what's happening. :-O > -Sara - Matt --- ## #86569 — Dmitry Stogov — 2015-06-10T17:57:07+00:00 URL: https://externals.io/message/86158#86569 · In reply to: #86552 > On Tue, Jun 9, 2015 at 6:05 AM, Matt Wilmas > wrote: > > […] > http://git.php.net/?p=php-src.git;a=commitdiff;h=c09698753e7d1d95299dca54c8ca888c885fd45b > > […] > Dmitry, what's the reasoning behind this diff in the first place? > Inlining of fast path and omitting check for Z_TYPE_P() for IS_CONST operands. > Doesn't the compiler fold ( . ) already > anyhow? How would we wind up with CONCAT_CONST_CONST at runtime? > It's probably makes sense to try to do this for most binary/unary opcodes. I'll think about it. > > > […] > it > > […] > I'm 90% certain that fixing the ordering (by saving to a temp var) > doesn't *actually* result in extra instructions. GCC's optimizer is > pretty clever. It'll notice you're only writing to the temp var once, > and reading from it once (immediately), so it'll end up organizing theIt > won't take more than a half an hour. > > instructions in a way which satisfies the explicit order, but as an > inline call. > I also think, this won't affect performance, and I may check this, creating a patch for all binary operators. Anyway, I don't object against this change, but I think, the order of operands evaluation must not be defined (by PHP specification). Thanks. Dmitry. > > -Sara --- ## #86570 — Dmitry Stogov — 2015-06-10T18:02:12+00:00 URL: https://externals.io/message/86158#86570 · In reply to: #86555 > > > […] > Okay, answered my own question (I need to be gentler with the [Send] > button). > > Zend/zend_compile.c sets up an assumption: > > if (left_node.op_type == IS_CONST) { > convert_to_string(&left_node.u.constant); > } > if (right_node.op_type == IS_CONST) { > convert_to_string(&right_node.u.constant); > } > > So any const nodes passed to CONCAT will always be strings (having > been preconverted), and the if check using an OR make sense, because > if it's CONST, then we KNOW it's a string, and there's no point > testing for it. We won't ever reach CONCAT_CONST_CONST (since that > would be folded in the compiler), but we might reach CONCAT_CONST_CV > or CONCAT_TMPVAR_CONST or some other combination thereof for which > this case is set to handle. > exectly. anyway CONCAT_CONST_CONST may be removed at all, at least to reduce the code size. TL;DR - Never mind me. I just didn't think it all the way through. > > -Sara > > P.S. - An assert(Z_TYPE_P(op1) == IS_STRING); and assert(Z_TYPE_P(op2) > == IS_STRING); does seem reasonable though... Maybe even with a > comment referencing that we can make that assumption because the > compiler fixes up the types. > ZEND_ASSERT() would be better. Thanks. Dmitry. --- ## #86572 — Dmitry Stogov — 2015-06-10T18:32:16+00:00 URL: https://externals.io/message/86158#86572 · In reply to: #86570 Hi Sara, https://gist.github.com/dstogov/6a90601872b538d2ddd6 I see no problems committing this (running tests now). Thanks. Dmitry. > > > > > > […] > > exectly. > anyway CONCAT_CONST_CONST may be removed at all, at least to reduce the > code size. > > TL;DR - Never mind me. I just didn't think it all the way through. > > […] > > ZEND_ASSERT() would be better. > > Thanks. Dmitry. --- ## #86573 — Matt Wilmas — 2015-06-10T18:45:30+00:00 URL: https://externals.io/message/86158#86573 · In reply to: #86572 Hi Dmitry, ----- Original Message ----- From: "Dmitry Stogov" Sent: Wednesday, June 10, 2015 > Hi Sara, > > https://gist.github.com/dstogov/6a90601872b538d2ddd6 > > I see no problems committing this (running tests now). Cool. :-) Just FYI, I noticed you didn't update ZEND_BOOL_XOR, at least... > Thanks. Dmitry. - Matt --- ## #86574 — Dmitry Stogov — 2015-06-10T18:58:31+00:00 URL: https://externals.io/message/86158#86574 · In reply to: #86573 and also forgot _DEREF suffix in IDENTICAL/NOT_IDENTICAL opcodes. :( Thanks. Dmitry. On Wed, Jun 10, 2015 at 9:45 PM, Matt Wilmas wrote: > Hi Dmitry, > > ----- Original Message ----- > From: "Dmitry Stogov" > Sent: Wednesday, June 10, 2015 > > Hi Sara, > > […] > > Cool. :-) Just FYI, I noticed you didn't update ZEND_BOOL_XOR, at least... > > Thanks. Dmitry. > > […] > > - Matt