I've got a one-line fix to allow internal functions to return by reference.
What happens currently is if (*return_value_ptr)->refcount <= 2 at the end
of an internal function, then the _get_zval_ptr_ptr_var delrefs it down to
refcount==1 (removing is_ref in the process). With userspace functions
ASSIGN_REF can handle this since
EX_T(opline->result.u.var).var.fcall_returned_reference is set appropriately
in fcall_common_helper. With internal functions, this value is never set.
The following one-liner change to Zend/zend_vm_def.h sets this value if the
function has declared itself to return reference (based on arg_info data),
and the value in return_value_ptr is in fact set to is_ref.
Two question: (1) Is it okay for me to just commit this (Andi? Zeev?), (2)
Should this fix be applied to 5.1 and 5.0 branches as well? Note: It
doesn't directly apply to 4.x branches since they have no return_value_ptr,
nor arg_info hinting. I havn't actually looked at the PHP_4_4 branch to see
if it has a similar problem.
-Sara
Index: zend_vm_def.h
RCS file: /repository/ZendEngine2/zend_vm_def.h,v
retrieving revision 1.68
diff -u -r1.68 zend_vm_def.h
--- zend_vm_def.h 19 Aug 2005 13:20:14 -0000 1.68
+++ zend_vm_def.h 23 Aug 2005 18:38:28 -0000
@@ -1883,6 +1883,8 @@
EX_T(opline->result.u.var).var.ptr->refcount = 1;
}
*/
-
EX_T(opline->result.u.var).var.fcall_returned_reference =
EX(function_state).function->common.return_reference &&
EX_T(opline->result.u.var).var.ptr->is_ref;
-
if (!return_value_used) { zval_ptr_dtor(&EX_T(opline->result.u.var).var.ptr); }
We'll take a look at it as this should actually work today. As to
back porting, I don't think we should touch the stable branches.
Andi
At 11:47 AM 8/23/2005, Sara Golemon wrote:
I've got a one-line fix to allow internal functions to return by reference.
What happens currently is if (*return_value_ptr)->refcount <= 2 at the end
of an internal function, then the _get_zval_ptr_ptr_var delrefs it down to
refcount==1 (removing is_ref in the process). With userspace functions
ASSIGN_REF can handle this since
EX_T(opline->result.u.var).var.fcall_returned_reference is set appropriately
in fcall_common_helper. With internal functions, this value is never set.The following one-liner change to Zend/zend_vm_def.h sets this value if the
function has declared itself to return reference (based on arg_info data),
and the value in return_value_ptr is in fact set to is_ref.Two question: (1) Is it okay for me to just commit this (Andi? Zeev?), (2)
Should this fix be applied to 5.1 and 5.0 branches as well? Note: It
doesn't directly apply to 4.x branches since they have no return_value_ptr,
nor arg_info hinting. I havn't actually looked at the PHP_4_4 branch to see
if it has a similar problem.-Sara
Index: zend_vm_def.h
RCS file: /repository/ZendEngine2/zend_vm_def.h,v
retrieving revision 1.68
diff -u -r1.68 zend_vm_def.h
--- zend_vm_def.h 19 Aug 2005 13:20:14 -0000 1.68
+++ zend_vm_def.h 23 Aug 2005 18:38:28 -0000
@@ -1883,6 +1883,8 @@
EX_T(opline->result.u.var).var.ptr->refcount = 1;
}
*/
EX_T(opline->result.u.var).var.fcall_returned_reference =EX(function_state).function->common.return_reference &&
EX_T(opline->result.u.var).var.ptr->is_ref;
if (!return_value_used) { zval_ptr_dtor(&EX_T(opline->result.u.var).var.ptr); }
We'll take a look at it as this should actually work today.
That's what I'd thought coming into this, but all my attempts to return a
reference failed:
My Arg Info struct:
static
ZEND_BEGIN_ARG_INFO_EX(php_sample_retref_arginfo, 0, 1, 0)
ZEND_END_ARG_INFO()
My reference linking:
if (return_value_ptr) {
zval_ptr_dtor(return_value_ptr);
}
SEPARATE_ZVAL_TO_MAKE_IS_REF(&val);
ZVAL_ADDREF(val);
*return_value_ptr = val;
There's some commented out code in there that refers to bug 34045:
/* We shouldn't fix bad extensions here,
because it can break proper ones (Bug #34045)
if (!EX(function_state).function->common.return_reference) {
EX_T(opline->result.u.var).var.ptr->is_ref = 0;
EX_T(opline->result.u.var).var.ptr->refcount = 1;
}
*/
With that code in there returning references would certainly never work so
I'd be curious about the events leading up to that removal as much as
anything.
As to back porting, I don't think we should touch the stable branches.
Eh, it's a 50/50 on this one since the bug only effects internals code and
(last I checked) there's nothing internal that's trying to return a
reference other that OOP code (because it gets shoved into being a reference
anyway). I only came across this because I was trying to come up with some
example code "If you wanted to do this you could...".
I know I personally would like to see it work in 5.1 at the least (being shy
of final such as we are...). As for 5.0... yeah let's sleeping dogs lie.
-Sara
OK thanks for the additional info. Will take a look.
At 05:05 PM 8/23/2005, Sara Golemon wrote:
We'll take a look at it as this should actually work today.
That's what I'd thought coming into this, but all my attempts to return a
reference failed:My Arg Info struct:
static
ZEND_BEGIN_ARG_INFO_EX(php_sample_retref_arginfo, 0, 1, 0)
ZEND_END_ARG_INFO()My reference linking:
if (return_value_ptr) {
zval_ptr_dtor(return_value_ptr);
}
SEPARATE_ZVAL_TO_MAKE_IS_REF(&val);
ZVAL_ADDREF(val);
*return_value_ptr = val;There's some commented out code in there that refers to bug 34045:
/* We shouldn't fix bad extensions here,
because it can break proper ones (Bug #34045)
if (!EX(function_state).function->common.return_reference) {
EX_T(opline->result.u.var).var.ptr->is_ref = 0;
EX_T(opline->result.u.var).var.ptr->refcount = 1;
}
*/With that code in there returning references would certainly never work so
I'd be curious about the events leading up to that removal as much as
anything.As to back porting, I don't think we should touch the stable branches.
Eh, it's a 50/50 on this one since the bug only effects internals code and
(last I checked) there's nothing internal that's trying to return a
reference other that OOP code (because it gets shoved into being a reference
anyway). I only came across this because I was trying to come up with some
example code "If you wanted to do this you could...".I know I personally would like to see it work in 5.1 at the least (being shy
of final such as we are...). As for 5.0... yeah let's sleeping dogs lie.-Sara
There's some commented out code in there that refers to bug 34045:
/* We shouldn't fix bad extensions here,
because it can break proper ones (Bug #34045)
if (!EX(function_state).function->common.return_reference) {
EX_T(opline->result.u.var).var.ptr->is_ref = 0;
EX_T(opline->result.u.var).var.ptr->refcount = 1;
}
*/With that code in there returning references would certainly never work so
I'd be curious about the events leading up to that removal as much as
anything.
Doi... Ignore that particular comment. All that code does is block
reference passing when the arg info isn't set. (Which would seem perfectly
reasonable...)
The rest of it still stands, I just spent too long unfolding engine code
today decoding the fcall_returned_reference logic... :)
-Sara