Fix #63217: Constant numeric strings become integers when used as ArrayAccess offset #3351

rtheunissen · 2018-06-28T23:03:27Z

This is a refresh of #2607, from which I'm quoting:

See:

https://bugs.php.net/bug.php?id=63217

https://3v4l.org/dagUP

https://externals.io/thread/1005

This PR is an extension and re-opening of #1649 in an attempt to fix this as part of 7.2. I've rebased on master and updated some tests that are failing. @nikic mentioned in #1649 that further changes are necessary but I'm not sure what they are or why they are necessary.

This patch does not affect the array or string API and behaviour at all - only ArrayAccess.

KalleZ · 2018-06-28T23:39:43Z

ext/opcache/Optimizer/zend_optimizer.c

+			// 		zval_ptr_dtor_nogc(val);
+			// 		ZVAL_LONG(val, index);
+			// 	}
+			// }


Minor note, as per our coding standards this should be a multi-line C style comment

Oh yeah, that's just a temporary removal, didn't mean to commit that. I'll remove the commented code entirely. Thanks. 😊

Done in 7aceef6

rtheunissen · 2018-06-29T03:03:30Z

If we can technically fix the behaviour, even if it's not in an optimal way, shouldn't that be a solid place to start for 7.3.0? I doubt that the current patch will affect performance enough to be a deal-breaker. It's an optimisation that affects a very rare case, as I have never come across PHP code where an array is accessed using a numeric string literal like $array['123'].

Personal opinion is that this should be merged so that we can fix the bug at hand, and worry about the tiny performance uncertainty later if there's no clear solution for that right now.

Correct behaviour should come before performance every time.

nikic · 2018-06-29T08:27:54Z

Zend/zend_vm_def.h

-				if (ZEND_HANDLE_NUMERIC(str, hval)) {
-					ZEND_VM_C_GOTO(num_index);
-				}
+			if (ZEND_HANDLE_NUMERIC(str, hval)) {


ADD_ARRAY_ELEMENT doesn't need to be touched (you also left the handle_numeric_op in the compiler).

Will revert changes to ADD_ARRAY_ELEMENT in 135449d, but zend_handle_numeric_op is still called in zend_compile_array. Should it not be? Andrea only removed one of the calls in the original PR.

No, it's right. I was just noting the inconsistency between the compiler and the runtime. The way you reserved it is correct.

nikic · 2018-06-29T08:29:05Z

ext/opcache/Optimizer/zend_inference.c

-					// FIXME: numeric string
-					tmp |= MAY_BE_ARRAY_KEY_LONG;
-				}
+				tmp |= (MAY_BE_ARRAY_KEY_LONG | MAY_BE_ARRAY_KEY_STRING);


These need to be changed to replace the dim_op_type != IS_CONST check with a ZEND_HANDLE_NUMERIC for CONST operands.

It would make sense to extract the whole key type checking into a separate function, as this code is repeated a couple of times.

You'll need to pass in the znode_op* for the dim next to dim_op_type.

So if dim_op_type == IS_CONST, we should do a ZEND_HANDLE_NUMERIC to add MAY_BE_ARRAY_KEY_LONG?

I'm really just following your lead here but I'll try to make sense of this. If the dimension operand is a constant (I'm guessing a literal string in our case), we should check if it's numeric.. but why? What is tmp here and who are we signalling that tmp MAY_BE_ARRAY_KEY_LONG?

Also, what's a znode_op?

If dim_op_type != IS_CONST: You always add MAY_BE_ARRAY_KEY_LONG.
If dim_op_type == IS_CONST: You do a ZEND_HANDLE_NUMERIC and only add MAY_BE_KEY_LONG if it's numeric.

To avoid the need to pass in the full kitchen sink of parameters to this function, you might want to pass opline->op2_type == IS_CONST ? CRT_CONSTANT_EX(op_array, opline, opline->op2, ssa->rt_constants) : NULL from the caller, rather than just opline->op2 (which is the znode_op I was referring to).

nikic · 2018-06-29T08:33:18Z

ext/opcache/Optimizer/zend_optimizer.c

-					zval_ptr_dtor_nogc(val);
-					ZVAL_LONG(val, index);
-				}
-			}


This code can be left for ADD_ARRAY_ELEMENT, and the rest can simply be dropped from the switch. They'll fall through to the default case.

Done in 6a4ae15 and ae6ba00

nikic · 2018-06-29T08:36:04Z

I doubt that the current patch will affect performance enough to be a deal-breaker. It's an optimisation that affects a very rare case, as I have never come across PHP code where an array is accessed using a numeric string literal like $array['123'].

The case that we're concerned about are not accesses like $array['123'], but rather $array['foo']. These will now have to go through the numeric string checking. However, in most cases that should just be a single branch, so this should not be too problematic (a micro benchmark is needed though, to verify assumptions).

rtheunissen · 2018-06-29T12:48:42Z

The case that we're concerned about are not accesses like $array['123'], but rather $array['foo']. These will now have to go through the numeric string checking. However, in most cases that should just be a single branch, so this should not be too problematic (a micro benchmark is needed though, to verify assumptions).

Of course, that makes sense. In most cases these would fail at the first character anyway. I'll do a proper micro-benchmark when the patch is more complete. I'll use this one that @hikari-no-yume wrote in #1649 (comment)

Thanks for your help again on this one @nikic

nikic · 2018-06-30T18:20:01Z

ext/opcache/Optimizer/zend_inference.c

@@ -2496,7 +2515,8 @@ static int zend_update_type_info(const zend_op_array *op_array,

 			if (opline->extended_value == ZEND_ASSIGN_DIM) {
 				if (opline->op1_type == IS_CV) {
-					orig = assign_dim_result_type(orig, OP2_INFO(), tmp, opline->op2_type);
+					orig = assign_dim_result_type(orig, OP2_INFO(), tmp, opline->op2_type, 
+						CRT_CONSTANT_EX(op_array, opline, opline->op2, ssa->rt_constants));


These CRT_CONSTANT_EX accesses should be guarded by opline->op2_type == IS_CONST.

See 79a14f5 and b0074a6

nikic · 2018-06-30T20:18:56Z

For this benchmark: https://gist.github.com/nikic/f80155ecc86fd4e48be50af9c35f64be I get the following results:

Test	Master	This PR
`$array["non_numeric"]`	3.3779599666595s	3.3726971149445s
`$array["!!!!!!!!!!!"]`	3.3831338882446s	3.3914439678192s
`$array["123456789"]`	2.5053958892822s	4.4804201126099s
`$array["123456789123456789123456789123456789"]`	3.9566950798035s	4.3563148975372s

The good news is that it seems to have negligible impact on the common case of non-numeric constant keys. The bad news is that cases like $array["123"] become a good bit slower, here by a factor of 1.8. However, as this should be a rather unusual case, I think that's fine.

From my side this is good to go. @dstogov What do you think?

rtheunissen · 2018-06-30T21:59:39Z

@nikic so that case is slow because it has to do the string-to-long conversion for every loop at runtime? Could we not do that conversion once, if we know that it's an array (can we know?)

nikic · 2018-06-30T22:29:01Z

ext/opcache/Optimizer/zend_inference.c

+				if (ZEND_HANDLE_NUMERIC(Z_STR_P(dim_op), hval)) {
+					tmp |= MAY_BE_ARRAY_KEY_LONG;
+				}
+			}


It would be better to write this as:

if (dim_type & MAY_BE_STRING) { if (dim_op_type != IS_CONST) { tmp |= MAY_BE_ARRAY_KEY_STRING | MAY_BE_ARRAY_KEY_LONG; } else { zend_ulong hval; if (ZEND_HANDLE_NUMERIC(Z_STR_P(dim_op), hval)) { tmp |= MAY_BE_ARRAY_KEY_LONG; } else { tmp |= MAY_BE_ARRAY_KEY_STRING; } } }

Otherwise we're adding MAY_BE_ARRAY_KEY_STRING even if we know numeric conversion will happen.

Good call. 👍 Done in 45e6b24

nikic · 2018-06-30T22:31:59Z

@rtheunissen Yes, we could do that for the cases where we can determine that it is an array. This would take an extra check in https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/dfa_pass.c#L866, which for all relevant opcodes checks if op1 has only MAY_BE_ARRAY type and op2 is a numeric string literal, and converts it in that case.

I don't think this is strictly necessary to land this though, as I don't think it's a common case.

dstogov · 2018-07-02T11:42:29Z

@nikic @rtheunissen I don't object.

…l behaviour

…ate_op2_const

nikic · 2018-07-02T14:44:29Z

Merged as 30156d5. Thanks!

dstogov · 2018-07-02T15:33:28Z

@nikic I got an idea, how this may be fixed without performance degradation. We may keep two constant literals (one of them should be converted to to number if necessary). This is similar to ZEND_GET_FUNC_BY_NAME that receives original and lowercase name. What do you think?

dstogov · 2018-07-02T15:44:14Z

ideally, it would be great to have second literal only for numeric string indexes (this will probably require some new flag).

nikic · 2018-07-02T15:48:34Z

@dstogov I think this case is too rare to make an extra literal worthwhile.

dstogov · 2018-07-02T15:58:39Z

@nikic $mysql_result["filed_name"] is quite common code. And yes, extra literal should be used only for numeric string constants.

nikic · 2018-07-02T16:02:05Z

@dstogov Yes, that is common code, but that case is not affected. Or rather, it will only perform one extra branch to check that 'f' > '9'. Based on my microbenchmark, this did not produce a measurable difference.

The case that has become slower is $array["123"], but this will usually be written as $array[123] instead, which is why I'm not concerned much.

hikari-no-yume · 2018-07-02T16:02:18Z

Potentially crackpot idea: what if "123" had a hash value of 123?

dstogov · 2018-07-02T16:16:32Z

@nikic you don't see performance difference, because on micro-benchmarks all the code and data are already in CPU cache and the extra branch is well predicted. Of course, the degradation from this patch is not significant, but if we can fix the problem in a better way, we should try.

@hikari-no-yume changing hash_value function may cause much more significant performance difference

nikic · 2018-07-02T16:21:17Z

@dstogov That's true. Adding an opline flag for this will still leave the branch though, unless we want to specialize it away. Do you have a numbers for the impact of this change on something like WP?

dstogov · 2018-07-02T17:15:39Z

@nikic

$ ZEND_DONT_UNLOAD_MODULES=1 valgrind --tool=callgrind --separate-recs=1 --dump-instr=yes --cache-sim=no sapi/cgi/php-cgi -T100 /var/www/html/bench/wordpress/index.php > /dev/null

before: 1,987M
after: 1,992M

I'll try to do a better fix myself, later today or tomorrow, May be I'll able to avoid extra flag in opline->execute_data, by using extra space in first zval instead. And we'll have to check it only in case of objects (arrays will use the old specialization logic).

KalleZ reviewed Jun 28, 2018

View reviewed changes

rtheunissen mentioned this pull request Jun 29, 2018

Fix #63217: Constant numeric strings become integers when used as ArrayAccess offset #2607

Closed

nikic reviewed Jun 29, 2018

View reviewed changes

rtheunissen force-pushed the rt-fix-bug-63217 branch 3 times, most recently from e0f09e7 to c30f56c Compare June 30, 2018 18:06

nikic reviewed Jun 30, 2018

View reviewed changes

rtheunissen force-pushed the rt-fix-bug-63217 branch from 45e6b24 to 0fb9aa7 Compare July 1, 2018 22:24

rtheunissen added 12 commits July 2, 2018 08:33

Add test for bug #63217

28b05be

Slight improvements to existing tests

fda2a17

Changes based on old PR in an attempt to achieve the correct technica…

fdcd64f

…l behaviour

Remove commented code

600cd03

Add small regression check

f1b3791

Fix small mistake in updated test

a4d1a4c

Patch a few more numeric string checks in zend_inference

5f17ce6

Revert changes to ADD_ARRAY_ELEMENT

e126849

Restore optimisation for ZEND_ADD_ARRAY_ELEMENT in zend_optimizer_upd…

86d9309

…ate_op2_const

Replace missing zend_optimizer_add_literal

9c39782

Fix numeric string check in zend_inference

d23855f

Potentially correct mixup between op1_typ1 and op2_type

9bc0817

rtheunissen added 4 commits July 2, 2018 08:33

Add small ArrayObject regression test

dbe5ec9

Guard CRT_CONSTANT_EX with opline->op2_type == IS_CONST, first pass

6680d6e

Move long CRT call to macro

6fa43a6

Slightly better inference for array key numeric check

ff078f8

rtheunissen force-pushed the rt-fix-bug-63217 branch from 0fb9aa7 to ff078f8 Compare July 2, 2018 12:34

nikic added the Bug label Jul 2, 2018

nikic closed this Jul 2, 2018

rtheunissen deleted the rt-fix-bug-63217 branch July 2, 2018 17:28

dktapps mentioned this pull request Jul 15, 2018

Updates for PHP 7.3 krakjoe/pthreads#882

Merged

Fix #63217: Constant numeric strings become integers when used as ArrayAccess offset #3351

Fix #63217: Constant numeric strings become integers when used as ArrayAccess offset #3351

Uh oh!

Conversation

rtheunissen commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtheunissen commented Jun 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtheunissen Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtheunissen Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Jun 29, 2018

Uh oh!

rtheunissen commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtheunissen commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Jun 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstogov commented Jul 2, 2018

Uh oh!

nikic commented Jul 2, 2018

Uh oh!

dstogov commented Jul 2, 2018

Uh oh!

dstogov commented Jul 2, 2018

Uh oh!

nikic commented Jul 2, 2018

Uh oh!

dstogov commented Jul 2, 2018

Uh oh!

nikic commented Jul 2, 2018

Uh oh!

hikari-no-yume commented Jul 2, 2018

Uh oh!

dstogov commented Jul 2, 2018

Uh oh!

nikic commented Jul 2, 2018

Uh oh!

dstogov commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rtheunissen Jun 29, 2018 •

edited

Loading

rtheunissen Jun 29, 2018 •

edited

Loading

rtheunissen commented Jun 29, 2018 •

edited

Loading

nikic commented Jun 30, 2018 •

edited

Loading

nikic commented Jun 30, 2018 •

edited

Loading

dstogov commented Jul 2, 2018 •

edited

Loading