Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: list() reference syntax - bug 6768 & 7930 #2371

Closed
wants to merge 11 commits into from
Closed

Conversation

@bp1222
Copy link
Contributor

bp1222 commented Feb 6, 2017

RFC: https://wiki.php.net/rfc/list_reference_assignment

Proposed implementation would expand list() assignment syntax to allow users to include reference assignments as part of the list() rather than needing to do independently.

break;
}
prev_op->extended_value = ZEND_LIST_MAKE_WRITABLE;
} while ((prev_op = (prev_op - 1)) != NULL);

This comment has been minimized.

Copy link
@nikic

nikic Feb 6, 2017

Member

Does this correctly handle the case of [[$a, &$b]] = $c, i.e. where the FETCH_LIST for $c[0] is not immediately before the FETCH_LIST for $c[0][1]?

This comment has been minimized.

Copy link
@nikic

nikic Feb 6, 2017

Member

In a similar vein, if $c = [] in that example, would a notice be generate for access of $c[0] or not? One of the variables is a simple read, while the other isn't.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 6, 2017

Author Contributor

To the former concern. Yes, thanks for the close eyes looking at that. I'll change around the loop there, going back looking for List related assign & fetch, to properly run the write up the list tree. Just pushed a change for it now.

continue;
}
break;
} else if (prev_op->opcode == ZEND_ASSIGN_REF || prev_op->opcode == ZEND_ASSIGN) {

This comment has been minimized.

Copy link
@nikic

nikic Feb 6, 2017

Member

Keep in mind that the list() keys may contain complex expressions that emit additional opcodes, e.g. [$a, $b . $c => $d] = $e is valid, in which case there will be a CONCAT before the FETCH_LIST.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 6, 2017

Author Contributor

Hmm, yes. I'm just trying to limit how far back we need to look. Obviously this is for optimization purposes here, but since it's during compile and not execute, I guess I could drop that check and just keep looking back through this op-stack. Not exactly efficient, but not sure if that's better or worse than keeping looking back through the op-array

This comment has been minimized.

Copy link
@nikic

nikic Feb 6, 2017

Member

However, once you handle that you will run into the larger problem that keeping INDIRECT variables alive across arbitrary expressions is not memory safe, because the expression might invalidate the location the INDIRECT points to. For example, something like [[&$b, $a[] = 1 => &$c]] = $a might reallocate the $a array during $a[] = 1.

This comment has been minimized.

Copy link
@nikic

nikic Feb 6, 2017

Member

Instead of fixing up opcodes after the fact you can do an up-front pass through the list() ast and add a flag for all list()s that recursively contain a reference. Then you can directly emit the opcode with the correct flag.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

It may not be perfect, but I changed it to be a pre-look that for each elem it'd look down the list seeing if there's references so we can set flag accordingly.

As to the next issue with indirect, I'm not sure how to get a case in which it fails.

$a = [
    1 => "one",
    2 => "two",
];

[[1 => &$b, $a[] = 2 => $c]] = $a;
var_dump($a);

outputs

PHP Notice:  Undefined offset: 0 in /home/dwalker/src/php/e.php on line 8
PHP Notice:  Undefined offset: 1 in /home/dwalker/src/php/e.php on line 8
PHP Notice:  Undefined offset: 2 in /home/dwalker/src/php/e.php on line 8
array(4) {
  [1]=>
  string(3) "one"
  [2]=>
  string(3) "two"
  [0]=>
  array(1) {
    [1]=>
    &NULL
  }
  [3]=>
  int(2)
}

Which is what I'm kind of expecting to see. The $a main array remains after the run. There's a 0'th element that is an array with the reference to NULL, and the odd key of $a[] = 2 does give a new element in $a with the value 2. If there some trick to make the indirect fail better to try and understand it better.

@nikic nikic added the RFC label Feb 6, 2017
#define ZEND_RETURN_VAL 0
#define ZEND_RETURN_REF 1
#define ZEND_RETURN_VALUE 0
#define ZEND_RETURN_REFERENCE 1

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 7, 2017

Member

This is unrelated change. It's better to commit it separately with appropriate comment.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

Will do, sorry old habits die hard.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

Before I do/revert spacing issues, question of best-practice for PR's. Interactive rebase, and remove the changes from prior commits, and force-update. Or, a commit to revert the spacing changes, and then re-commit spacing changes after?

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 8, 2017

Member

I think, better ti do a squash commit at the end.

@@ -1622,7 +1622,7 @@ static zend_always_inline zval *zend_fetch_dimension_address_inner(HashTable *ht
switch (Z_TYPE_P(dim)) {
case IS_UNDEF:
zval_undefined_cv(EG(current_execute_data)->opline->op2.var, EG(current_execute_data));
/* break missing intentionally */
/* break missing intentionally */

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 7, 2017

Member

Please, don't mix white space changes with semantic patches. commit this separately.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

Again, old habits, will revert.

@@ -1881,7 +1881,7 @@ static zend_always_inline void zend_fetch_dimension_address_read(zval *result, z
dim = &EG(uninitialized_zval);
}
if (!Z_OBJ_HT_P(container)->read_dimension) {
zend_throw_error(NULL, "Cannot use object as array");
zend_throw_error(NULL, "Cannot use object of type %s as array", ZSTR_VAL(Z_OBJCE_P(container)->name));

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 7, 2017

Member

This is also unrelated. I'm not sure, if we need this.

ZVAL_REF(EX_VAR(opline->result.var), Z_REF_P(retval_ptr));
} else {
ZVAL_COPY_VALUE(EX_VAR(opline->result.var), &retval);
}

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 7, 2017

Member

Instead of FETCH_LIST modification and relaying on extended_value, it's better to rename unmodified FETCH_LIST into FETCH_LIST_R and introduce new FETCH_LIST_RW, that accepts only CV and VAR as op1 . This should make the patch completely safe.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

I was debating doing that as well. However, since I didn't want to get too expansive on, what seemed to be a smaller change (w/ indirects earlier) I kept the one. I will split this into the two respective opcodes.

When I do that, my other concern, was for opcode caching. I'm not overly familiar with that end of the code, and am not sure the direct impact that would have there. Would you be able to shed any light on the implications of making FETCH_LIST_R/RW?

zend_emit_assign_ref_znode(var_ast, &fetch_result);
} else {
zend_emit_assign_znode(var_ast, &fetch_result);
}

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 7, 2017

Member

expr_node might be IS_CONST or IS_TMP_VAR.

These expressions should be prohibited in the same way as $a =& 5; or $a =& $x + $y;

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

Thanks, will test and check for it as well.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

The $a =& 5 is prohibited as a parse error, whereas shockingly $a =& $x+$y is executed, albeit I concur it probably shouldn't be.

<?php
$a = 10;
$b = 20;

$c =& $a + $b;

outputs:
int(10)
int(20)
int(10)

However, wrapping it like $c =& ($a + $b) is also a parse error.

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 7, 2017

Author Contributor

However, constants, and tmpvars, appear to be caught during execution raising a Fatal, wherein assignments can only happen to writable values. So it's preventing the odd uses. I assume the check you're asking for here is a compile time check, rather than an execution based one.

This comment has been minimized.

Copy link
@nikic

nikic Feb 25, 2017

Member

Regarding $c =& $a + $b: PHP defines the =& operator as accepting variables on both sides, so this is parsed as ($c &= $a) + $b (as $a + $b is not a variable). This is similar to how !$x = f() is parsed as !($x = f()) rather than (!$x) = f(), as most other languages would interpret it.

@bp1222 bp1222 force-pushed the bp1222:fix-7930 branch from 1d2fb51 to c6d6ee4 Feb 7, 2017
@bp1222 bp1222 changed the title RFC: list() reference syntax - bug 7930 RFC: list() reference syntax - bug 6768 & 7930 Feb 7, 2017
{
zend_fetch_dimension_address_read(result, container, dim, IS_TMP_VAR, BP_VAR_R, 0, 0);
}

static zend_never_inline void zend_fetch_dimension_address_LIST_RW(zval *result, zval *container, zval *dim)
{
zend_fetch_dimension_address(result, container, dim, IS_TMP_VAR, BP_VAR_RW);

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 8, 2017

Member

Please, verify string offset handling.

$s = "abc"; [$a, &$b, $c]= $s; var_dump($a, $b, $c);

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 8, 2017

Author Contributor

I believe this is expected behavior, will include it in a test as well.

PHP Fatal error:  Uncaught Error: Cannot create references to/from string offsets in /home/dwalker/src/php/e.php:4
Stack trace:
#0 {main}
  thrown in /src/php/e.php on line 4

zend_fetch_dimension_address_LIST_R(&retval, container, GET_OP2_ZVAL_PTR_UNDEF(BP_VAR_R));

ZVAL_COPY_VALUE(EX_VAR(opline->result.var), &retval);

This comment has been minimized.

Copy link
@dstogov

dstogov Feb 8, 2017

Member

Why did you change this to perform double copy?

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Feb 22, 2017

RFC has been accepted.

@emilv emilv mentioned this pull request Feb 23, 2017
@emilv

This comment has been minimized.

Copy link

emilv commented Feb 23, 2017

There is no test case for ['one' => &$one, 'two' => $two].

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Feb 23, 2017

edit: ah, not using list() using [], I see, I can add.

@emilv - the tests for list_reference_array.phpt contains
$a = ['two' => 2, 'one' => 1];
list('one' => &$one, 'two' => $two) = $a;
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Feb 24, 2017

The Travis build is current failing (compile error).

if (elem_ast) {
zend_ast *var_ast = elem_ast->child[0];

if (elem_ast->kind == ZEND_AST_ARRAY_ELEM && elem_ast->attr) {

This comment has been minimized.

Copy link
@nikic

nikic Feb 24, 2017

Member

Can this ever be something other than an ARRAY_ELEM?

This comment has been minimized.

Copy link
@bp1222

bp1222 Feb 25, 2017

Author Contributor

I don't believe so, I tried thinking of what other sub-container of variables could exist within list() syntax. Or, at least, I'm not familiar with any other.

@@ -966,6 +966,8 @@ static zend_always_inline int zend_check_arg_send_type(const zend_function *zf,
#define ZEND_RETURNS_FUNCTION 1<<0
#define ZEND_RETURNS_VALUE 1<<1

#define ZEND_LIST_MAKE_WRITABLE 1

This comment has been minimized.

Copy link
@nikic

nikic Feb 24, 2017

Member

Looks like a leftover from a previous implementation.

container = GET_OP1_ZVAL_PTR_UNDEF(BP_VAR_RW);

if (UNEXPECTED(OP1_TYPE == IS_VAR && !Z_ISREF_P(container))) {
zend_error(E_NOTICE, "Attempting to set reference to non refereancable value");

This comment has been minimized.

Copy link
@nikic

nikic Feb 24, 2017

Member

nit: Typo "refereancable"

SAVE_OPLINE();
container = GET_OP1_ZVAL_PTR_UNDEF(BP_VAR_RW);

if (UNEXPECTED(OP1_TYPE == IS_VAR && !Z_ISREF_P(container))) {

This comment has been minimized.

Copy link
@nikic

nikic Feb 24, 2017

Member

This looks a bit heavy-handed to me. VARs that can be turned into refs are not necessarily already refs. Can you try if this works correctly on something like list(&$x) = $obj->x, where $obj->x is not already a reference?


if (UNEXPECTED(Z_TYPE(retval) == IS_INDIRECT)) {
retval_ptr = Z_INDIRECT(retval);
if (EXPECTED(!Z_ISREF_P(retval_ptr))) {

This comment has been minimized.

Copy link
@nikic

nikic Feb 24, 2017

Member

ZVAL_MAKE_REF already contains this condition.

@jwatzman

This comment has been minimized.

Copy link

jwatzman commented Feb 25, 2017

Apologies if I missed them, but I don't see test cases for some of the more perverse behaviour you can do with these sorts of assignments, e.g.,

list($a, &$a) = $arr;
list(&$a, $a) = $arr;
list($a, &$a) = $a;
list(&$a, $a) = $a;

Or what about:

$a = 1;
$b = &$a;
$arr = [&$a, &$b];
list(&$a, &$b) = $arr; // Or list(&$b, &$a) = $arr;

This sort of edge case having bizarre unintended behaviour has caused enormous grief to users and other runtime developers in the past (both HHVM and future-PHP) and so are probably worth making sure there aren't any hidden mines in here somewhere :)

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Feb 25, 2017

This PR doesn't seem to touch the code in https://github.com/php/php-src/blob/master/Zend/zend_compile.c#L3017, so the RHS of list() is still fetched in R mode, rather than RW. Pretty sure this means that if the RHS is anything but a simple variable the reference unpacking isn't going to work correctly. Would be good to have tests where the RHS is something other than a CV.

(Note that if it is fetched as RW, then there is again the INDIRECT safety issue mentioned earlier, so this would have to be converted into a reference proactively as well.)

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Feb 25, 2017

What also seems to be missing from the implementation is handling for destructuring in foreach, i.e. something like foreach ($coords as [&$x, &$y, &$z]). This should presumably switch to an RW foreach as well.

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Feb 25, 2017

@nikic - what would be a case of a RHS list()? I will add tests for something like this:

function a($a) {
    return $a;
}
$arr = [1,2];
list(&$a, $b) = a($arr);
var_dump($a, $b);

(fwiw, the above emits a PHP Notice: Attempting to set reference to non refereancable value in /home/dwalker/src/php/e.php on line 8

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Feb 25, 2017

Also, I'm not certain how to fix the travis compile error, the complaint about the ZEND_FETCH_LIST was changed in this PR to contain _R and _RW

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Feb 25, 2017

I'm not totally sure how this is happening. Current master contains a ZEND_FETCH_LIST reference there: https://github.com/php/php-src/blob/master/ext/opcache/Optimizer/zend_optimizer.c#L458 However, your tree does not, so I wouldn't expect this to cause a problem :/

Probably this will go away if you rebase onto master and replace the additional reference to ZEND_FETCH_LIST.

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Feb 25, 2017

Ok, I'm addressing other issues now (specifically your catch about

list(&$v) = $object->array_prop;
@nikic

This comment has been minimized.

Copy link
Member

nikic commented May 1, 2017

Any progress here?

@bp1222 bp1222 force-pushed the bp1222:fix-7930 branch 5 times, most recently from 5af885d to 5cf9a54 Oct 8, 2017
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Oct 28, 2017

This looks pretty good to me. There is one larger issue with regard to memory safety: The PHP VM has strict requirements about no user code executing between a FETCH_W value being created and used. See http://nikic.github.io/2017/04/14/PHP-7-Virtual-machine.html#writes-and-memory-safety for a brief explanation.

This issue currently arises for code like the following:

$ary = [[0, 1]];
[[
    0 => &$a,
    ($ary["foo"] = 1) => &$b
]] = $ary;

This produces the following opcodes:

function name: (null)
L1-8 {main}() /home/nikic/php-src/t085.php - 0x7f9423c82000 + 10 ops
 L3    #0     ASSIGN                  $ary                 array(1)                                 
 L7    #1     FETCH_LIST_W            $ary                 0                    @1                  
 L5    #2     FETCH_LIST_W            @1                   0                    @2                  
 L5    #3     ASSIGN_REF              $a                   @2                                       
 L6    #4     ASSIGN_DIM              $ary                 "foo"                @3                  
 L6    #5     OP_DATA                 1                                                             
 L6    #6     FETCH_LIST_W            @1                   @3                   @4                  
 L6    #7     ASSIGN_REF              $b                   @4                                       
 L6    #8     FREE                    @1                                                            
 L8    #9     RETURN<-1>              1                                                             

Notably, the @1 variable is created in # 1 and then used in # 6 and quite a lot of code runs in between. In this case the array is reallocated in the meantime, resulting in the following valgrind warnings:

==21437== Invalid read of size 1
==21437==    at 0xCBAB54: zval_get_type (zend_types.h:390)
==21437==    by 0xCC12CC: zend_fetch_dimension_address (zend_execute.c:1610)
==21437==    by 0xCC22C2: zend_fetch_dimension_address_LIST_w (zend_execute.c:1846)
==21437==    by 0xD5CFCB: ZEND_FETCH_LIST_W_SPEC_TMPVAR_TMPVAR_HANDLER (zend_vm_execute.h:53507)
==21437==    by 0xD61F2E: execute_ex (zend_vm_execute.h:59970)
==21437==    by 0xD621CD: zend_execute (zend_vm_execute.h:64034)
==21437==    by 0xC57E8B: zend_execute_scripts (zend.c:1498)
==21437==    by 0xB8E41B: php_execute_script (main.c:2451)
==21437==    by 0xD64D9E: do_cli (php_cli.c:1011)
==21437==    by 0xD66230: main (php_cli.c:1404)
==21437==  Address 0xfa393c0 is 16 bytes inside a block of size 264 free'd
==21437==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21437==    by 0xC11960: _efree (zend_alloc.c:2428)
==21437==    by 0xC6C1DD: zend_hash_packed_to_hash (zend_hash.c:216)
==21437==    by 0xC6D9B9: _zend_hash_add_or_update_i (zend_hash.c:537)
==21437==    by 0xC6DDF7: _zend_hash_add_new (zend_hash.c:620)
==21437==    by 0xCC1038: zend_fetch_dimension_address_inner (zend_execute.c:1549)
==21437==    by 0xCC1219: zend_fetch_dimension_address_inner_W_CONST (zend_execute.c:1593)
==21437==    by 0xD29FA5: ZEND_ASSIGN_DIM_SPEC_CV_CONST_OP_DATA_CONST_HANDLER (zend_vm_execute.h:37065)
==21437==    by 0xD61F2E: execute_ex (zend_vm_execute.h:59970)
==21437==    by 0xD621CD: zend_execute (zend_vm_execute.h:64034)
==21437==    by 0xC57E8B: zend_execute_scripts (zend.c:1498)
==21437==    by 0xB8E41B: php_execute_script (main.c:2451)
==21437==  Block was alloc'd at
==21437==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==21437==    by 0xC129DB: __zend_malloc (zend_alloc.c:2811)
==21437==    by 0xC117E5: _emalloc (zend_alloc.c:2413)
==21437==    by 0xC717D6: zend_array_dup (zend_hash.c:1772)
==21437==    by 0xCC1337: zend_fetch_dimension_address (zend_execute.c:1612)
==21437==    by 0xCC22C2: zend_fetch_dimension_address_LIST_w (zend_execute.c:1846)
==21437==    by 0xD27D5A: ZEND_FETCH_LIST_W_SPEC_CV_CONST_HANDLER (zend_vm_execute.h:36402)
==21437==    by 0xD61F2E: execute_ex (zend_vm_execute.h:59970)
==21437==    by 0xD621CD: zend_execute (zend_vm_execute.h:64034)
==21437==    by 0xC57E8B: zend_execute_scripts (zend.c:1498)
==21437==    by 0xB8E41B: php_execute_script (main.c:2451)
==21437==    by 0xD64D9E: do_cli (php_cli.c:1011)

An easy way to fix this is to emit an additional MAKE_REF instruction after each LIST_W. (Alternatively the logic could also be included in LIST_W.)

if (zend_compile_list_assign_requires_w(var_ast) && zend_is_variable(expr_ast)) {
zend_compile_var(&expr_node, expr_ast, BP_VAR_W);
} else {
zend_compile_expr(&expr_node, expr_ast);

This comment has been minimized.

Copy link
@nikic

nikic Oct 28, 2017

Member

I'm wondering if it wouldn't be better to throw a compile time error in this case. After all things like $foo =& 42 are parse errors, so I would expect list(&$foo) = [42] to be a compile error.

This comment has been minimized.

Copy link
@bp1222

bp1222 Oct 30, 2017

Author Contributor

I can change. I guess they are kind of synonymous, attempt to assign to a non referenceable value.

@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Oct 30, 2017

@nikic - Thanks for the explanation of the VM. (ps, enjoy your blog on the internals, super helpful). Added the ref, and valgrind is coming up clean locally, and added a test for your example case. Also flipped around so we can toss out a compiler error & added tests to the effect.

zend_compile_expr(&expr_node, expr_ast);
}
} else {
zend_error_noreturn(E_COMPILE_ERROR, "Cannot assign reference to non referencable value");

This comment has been minimized.

Copy link
@nikic

nikic Oct 30, 2017

Member

Looks like the branches are inverted here. Now this error will also be thrown for normal, non-reference list assignments.

This comment has been minimized.

Copy link
@bp1222

bp1222 Oct 30, 2017

Author Contributor

I neglected to run against more than just the list tests. Yes, it should be checking not if variable, but, if needs a write.

ZEND_VM_NEXT_OPCODE_CHECK_EXCEPTION();
}

ZEND_VM_HANDLER(198, ZEND_FETCH_LIST_W, CONST|TMPVAR|CV, CONST|TMPVAR|CV)

This comment has been minimized.

Copy link
@nikic

nikic Oct 30, 2017

Member

It should be possible drop the the CONST| for the first operand here now, as it is covered by the compile-time check.

zend_compile_expr(&expr_node, expr_ast);
if (zend_compile_list_assign_requires_w(var_ast)) {
if (zend_is_variable(expr_ast)) {
zend_compile_var(&expr_node, expr_ast, BP_VAR_W);

This comment has been minimized.

Copy link
@nikic

nikic Nov 2, 2017

Member

This case also needs an extra MAKE_REF opcode. In this case it may make sense to only emit it if expr_node.op_type != IS_CV.

This comment has been minimized.

Copy link
@bp1222

bp1222 Nov 7, 2017

Author Contributor

Would you possibly have an example of code that would utilize this case, or rather, would expose why it'd be necessary? So I can use it for understanding why / adding a test case.

This comment has been minimized.

Copy link
@nikic

nikic Nov 7, 2017

Member

Working off the previous test-case, something like this should trigger it (not tested):

$ary = [[0, 1]];
[
    0 => &$a,
    ($ary["foo"] = 1) => &$b
] = $ary[0];

So basically the case where an array access has been shifted from the LHS to the RHS of the list assignment.

@@ -3025,7 +3068,15 @@ void zend_compile_assign(znode *result, zend_ast *ast) /* {{{ */
/* list($a, $b) = $a should evaluate the right $a first */
zend_compile_simple_var_no_cv(&expr_node, expr_ast, BP_VAR_R, 0);
} else {

This comment has been minimized.

Copy link
@nikic

nikic Nov 2, 2017

Member

The zend_list_has_assign_to_self() branch above currently doesn't handle the reference case, it also needs to switch to BP_VAR_W (+ MAKE_REF).

This comment has been minimized.

Copy link
@bp1222

bp1222 Nov 8, 2017

Author Contributor

So, fwiw, zend_compile_simple_var_no_cv does seem to accept the BP_ type, but then proceeds to do nothing with that argument. Each emit in there is hardcoded for an _R type as well.

This comment has been minimized.

Copy link
@nikic

nikic Nov 16, 2017

Member

Sorry for the delay here. I've just pushed a tweak (9fbb019) so that zend_compile_simple_var_no_cv does not require an explicit zend_adjust_for_fetch_type call after it, so just passing the right fetch type should work now.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 2, 2017

For the opcache support, I think it should be sufficient to add ZEND_FETCH_LIST_W to all switches that currently contain ZEND_FETCH_DIM_W. I don't think there are any differences in how it needs to be handled.

David Walker and others added 2 commits Oct 6, 2017
Add unit tests

Update opcode name

Remove obsolete test

Apply changes requested
- Make _w fetch from list a ref
- Raise compiler error when assigning to a known non referencable value

Remove unused condition in VM

Fix logic for compile error, drop const from LIST_W op1

Add additional make ref, for write access

Add ZEND_FETCH_LIST_W to switches and places DIM_W exists
@bp1222 bp1222 force-pushed the bp1222:fix-7930 branch from 83c2d38 to 196dd1d Nov 20, 2017
@bp1222

This comment has been minimized.

Copy link
Contributor Author

bp1222 commented Nov 21, 2017

Tests are failing due to segfault, but I'm not able to replicate anywhere I'm building on. How does one best debug that?

nikic added 3 commits Nov 21, 2017
* Don't generate MAKE_REF for CVs normally
* But do generate it for self-assign CVs
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 21, 2017

@bp1222 These are opcache related failure. I've pushed 28157db to fix this, together with some other changes.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Nov 21, 2017

I don't see any more open issues here. @dstogov Would you like to review as well?

nikic added 5 commits Dec 9, 2017
Let assign_ref handle the MAKE_REF emission. Only explicitly emit
the MAKE_REF if it's for a (non-leaf) list() assignment.
Rather than recomputing it on every access, propagate only once and
store it in elem_ast->attr, consistently with direct references.
Make all compile_list_assign() calls explicit, instead of
automatically calling it from assign_znode(). I did not like the
asymmetry between assign_znode() and assign_ref_znode() here.
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Dec 9, 2017

Sorry for the delay, now merged as 6d4de4c into master. Thanks a lot for your work on this RFC!

@nikic nikic closed this Dec 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.