Skip to content

Restructure argument passing onto stack#1679

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.

Sign up for GitHub

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

Closed
bwoebi wants to merge 7 commits into from
Closed

Restructure argument passing onto stack #1679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89c2585
Restructure argument passing onto stack
bwoebi Dec 5, 2015
35d5225
Merge branch 'master' into vm_stack_restructuring
dstogov Dec 17, 2015
2a230dc
Move fcall/arg temp generation to zend_compile.c
bwoebi Dec 22, 2015
82d44e6
Add a simple optimization to remove nearly all ZEND_SEND_VAL_SPEC_TMP…
bwoebi Dec 22, 2015
98d9614
Merge branch 'master' of https://github.com/php/php-src into vm_stack…
bwoebi Dec 22, 2015
21e996f
Fix phpdbg tests, add handling for live_range when optimizing out SEN…
bwoebi Dec 22, 2015
6d46c57
Merge branch 'master' of https://github.com/php/php-src into vm_stack…
bwoebi Dec 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions Zend/tests/arg_unpack/invalid_type.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -16,19 +16,19 @@ test(1, 2, 3, ...new StdClass, ...3.14, ...[4, 5]);

?>
--EXPECTF--
Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 1 requires array or Traversable, null given in %s on line %d
array(0) {
}

Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 1 requires array or Traversable, integer given in %s on line %d
array(0) {
}

Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 1 requires array or Traversable, object of type stdClass given in %s on line %d
array(0) {
}

Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 4 requires array or Traversable, string given in %s on line %d
array(5) {
[0]=>
int(1)
Expand All@@ -42,9 +42,9 @@ array(5) {
int(5)
}

Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 4 requires array or Traversable, object of type stdClass given in %s on line %d

Warning: Only arrays and Traversables can be unpacked in %s on line %d
Warning: Argument unpacking at parameter 4 requires array or Traversable, float given in %s on line %d
array(5) {
[0]=>
int(1)
Expand Down
9 changes: 5 additions & 4 deletions Zend/zend_API.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -70,7 +70,7 @@ ZEND_API int zend_get_parameters(int ht, int param_count, ...) /* {{{ */
ZVAL_COPY_VALUE(param_ptr, &new_tmp);
}
*param = param_ptr;
param_ptr++;
param_ptr--;

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove deprecated API by separate commit: zend_get_parameters(), zend_get_parameters_ex(), zend_copy_parameters_array().

}
va_end(ptr);

Expand All@@ -97,7 +97,7 @@ ZEND_API int zend_get_parameters_ex(int param_count, ...) /* {{{ */
while (param_count-->0) {
param = va_arg(ptr, zval **);
*param = param_ptr;
param_ptr++;
param_ptr--;
}
va_end(ptr);

Expand All@@ -120,7 +120,7 @@ ZEND_API int _zend_get_parameters_array_ex(int param_count, zval *argument_array
while (param_count-->0) {
ZVAL_COPY_VALUE(argument_array, param_ptr);
argument_array++;
param_ptr++;
param_ptr--;

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zend_get_parameters_array() is going to be logical inconsistent with the rest or parameter parsing API.
It uses direct arguments order, while all others uses reverse order.
I think this may be solved by introducing simple arg_array API.
This should also solve mess with negative indexing and catch all affected places at compile time.
I think such arg_array API may be designed and applied before this patch.

}

return SUCCESS;
Expand All@@ -144,7 +144,7 @@ ZEND_API int zend_copy_parameters_array(int param_count, zval *argument_array) /
Z_ADDREF_P(param_ptr);
}
zend_hash_next_index_insert_new(Z_ARRVAL_P(argument_array), param_ptr);
param_ptr++;
param_ptr--;
}

return SUCCESS;
Expand DownExpand Up@@ -2158,6 +2158,7 @@ ZEND_API int zend_register_functions(zend_class_entry *scope, const zend_functio
internal_function->function_name = zend_new_interned_string(zend_string_init(ptr->fname, fname_len, 1));
internal_function->scope = scope;
internal_function->prototype = NULL;
internal_function->stack_size = ZEND_CALL_FRAME_SLOT * ZEND_MM_ALIGNED_SIZE(sizeof(zval));

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all internal functions use the same stack size?
I'm not sure, if we need to waste memory for each function.

if (ptr->flags) {
if (!(ptr->flags & ZEND_ACC_PPP_MASK)) {
if (ptr->flags != ZEND_ACC_DEPRECATED || scope) {
Expand Down
8 changes: 4 additions & 4 deletions Zend/zend_API.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -767,7 +767,7 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_wrong_callback_error(int severity, in
if (_optional) { \
if (UNEXPECTED(_i >_num_args)) break; \
} \
_real_arg++; \
_real_arg--; \
_arg = _real_arg; \
ZVAL_DEREF(_arg); \
if (separate) { \
Expand DownExpand Up@@ -1004,7 +1004,7 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_wrong_callback_error(int severity, in
ZEND_ASSERT(_i <= _min_num_args || _optional==1); \
ZEND_ASSERT(_i > _min_num_args || _optional==0); \
if (_optional && UNEXPECTED(_i >_num_args)) break; \
_real_arg++; \
_real_arg--; \
zend_parse_arg_zval(_real_arg, &dest, check_null); \
}

Expand All@@ -1023,10 +1023,10 @@ ZEND_API ZEND_COLD void ZEND_FASTCALL zend_wrong_callback_error(int severity, in
#define Z_PARAM_VARIADIC_EX(spec, dest, dest_num, post_varargs) do { \
int _num_varargs = _num_args - _i - (post_varargs); \
if (EXPECTED(_num_varargs > 0)) { \
dest = _real_arg + 1; \
dest = _real_arg - 1; \
dest_num = _num_varargs; \
_i += _num_varargs; \
_real_arg += _num_varargs; \
_real_arg -= _num_varargs; \
} else { \
dest = NULL; \
dest_num = 0; \
Expand Down
61 changes: 8 additions & 53 deletions Zend/zend_builtin_functions.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -438,10 +438,9 @@ ZEND_FUNCTION(func_num_args)
Get the $arg_num'th argument that was passed to the function */
ZEND_FUNCTION(func_get_arg)
{
uint32_t arg_count, first_extra_arg;
zval *arg;
zend_long requested_offset;
zend_execute_data *ex;
zend_execute_data *ex = EX(prev_execute_data);

if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &requested_offset) == FAILURE) {
return;
Expand All@@ -458,19 +457,12 @@ ZEND_FUNCTION(func_get_arg)
RETURN_FALSE;
}

arg_count = ZEND_CALL_NUM_ARGS(ex);

if (requested_offset >= arg_count) {
if (requested_offset >= ZEND_CALL_NUM_ARGS(ex)) {
zend_error(E_WARNING, "func_get_arg(): Argument " ZEND_LONG_FMT " not passed to function", requested_offset);
RETURN_FALSE;
}

first_extra_arg = ex->func->op_array.num_args;
if (requested_offset >= first_extra_arg && (ZEND_CALL_NUM_ARGS(ex) > first_extra_arg)) {
arg = ZEND_CALL_VAR_NUM(ex, ex->func->op_array.last_var + ex->func->op_array.T) + (requested_offset - first_extra_arg);
} else {
arg = ZEND_CALL_ARG(ex, requested_offset + 1);
}
arg = ZEND_CALL_ARG(ex, requested_offset + 1);
if (EXPECTED(!Z_ISUNDEF_P(arg))) {
ZVAL_DEREF(arg);
ZVAL_COPY(return_value, arg);
Expand All@@ -483,8 +475,7 @@ ZEND_FUNCTION(func_get_arg)
ZEND_FUNCTION(func_get_args)
{
zval *p, *q;
uint32_t arg_count, first_extra_arg;
uint32_t i, n;
uint32_t arg_count, i, n;
zend_execute_data *ex = EX(prev_execute_data);

if (ZEND_CALL_INFO(ex) & ZEND_CALL_CODE) {
Expand All@@ -496,29 +487,12 @@ ZEND_FUNCTION(func_get_args)

array_init_size(return_value, arg_count);
if (arg_count) {
first_extra_arg = ex->func->op_array.num_args;
zend_hash_real_init(Z_ARRVAL_P(return_value), 1);
ZEND_HASH_FILL_PACKED(Z_ARRVAL_P(return_value)) {
i = 0;
n = 0;
p = ZEND_CALL_ARG(ex, 1);
if (arg_count > first_extra_arg) {
while (i < first_extra_arg) {
q = p;
if (EXPECTED(Z_TYPE_INFO_P(q) != IS_UNDEF)) {
ZVAL_DEREF(q);
if (Z_OPT_REFCOUNTED_P(q)) {
Z_ADDREF_P(q);
}
n++;
}
ZEND_HASH_FILL_ADD(q);
p++;
i++;
}
p = ZEND_CALL_VAR_NUM(ex, ex->func->op_array.last_var + ex->func->op_array.T);
}
while (i < arg_count) {
do {

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need special cases for "extra" arguments handling. This is good!

q = p;
if (EXPECTED(Z_TYPE_INFO_P(q) != IS_UNDEF)) {
ZVAL_DEREF(q);
Expand All@@ -528,9 +502,9 @@ ZEND_FUNCTION(func_get_args)
n++;
}
ZEND_HASH_FILL_ADD(q);
p++;
p--;
i++;
}
} while (i < arg_count);
} ZEND_HASH_FILL_END();
Z_ARRVAL_P(return_value)->nNumOfElements = n;
}
Expand DownExpand Up@@ -2243,25 +2217,6 @@ static void debug_backtrace_get_args(zend_execute_data *call, zval *arg_array) /

zend_hash_real_init(Z_ARRVAL_P(arg_array), 1);
ZEND_HASH_FILL_PACKED(Z_ARRVAL_P(arg_array)) {
if (call->func->type == ZEND_USER_FUNCTION) {
uint32_t first_extra_arg = call->func->op_array.num_args;

if (ZEND_CALL_NUM_ARGS(call) > first_extra_arg) {
while (i < first_extra_arg) {
if (EXPECTED(Z_TYPE_INFO_P(p) != IS_UNDEF)) {
if (Z_OPT_REFCOUNTED_P(p)) {
Z_ADDREF_P(p);
}
n++;
}
ZEND_HASH_FILL_ADD(p);
p++;
i++;
}
p = ZEND_CALL_VAR_NUM(call, call->func->op_array.last_var + call->func->op_array.T);
}
}

while (i < num_args) {
if (EXPECTED(Z_TYPE_INFO_P(p) != IS_UNDEF)) {
if (Z_OPT_REFCOUNTED_P(p)) {
Expand All@@ -2270,7 +2225,7 @@ static void debug_backtrace_get_args(zend_execute_data *call, zval *arg_array) /
n++;
}
ZEND_HASH_FILL_ADD(p);
p++;
p--;
i++;
}
} ZEND_HASH_FILL_END();
Expand Down
3 changes: 2 additions & 1 deletion Zend/zend_closures.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,7 +147,7 @@ ZEND_METHOD(Closure, call)

fci.retval = &closure_result;
fci.params = my_params;
fci.param_count = my_param_count;
fci.param_count = -my_param_count;

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

negative param_count looks terrible.
Can we keep them positive?

@bwoebibwoebiDec 16, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small note: negative param count is a hack to avoid copying around. I'm open for other suggestions. (let's discuss that later.)

@dstogovdstogovDec 16, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you eliminate copy in comparison to php-7? (I didn't understand this)

On Wed, Dec 16, 2015 at 5:38 PM, Bob Weinand notifications@github.com
wrote:

In Zend/zend_closures.c
#1679 (comment):

@@ -147,7 +147,7 @@ ZEND_METHOD(Closure, call)

fci.retval = &closure_result;
fci.params = my_params;
  • fci.param_count = my_param_count;
  • fci.param_count = -my_param_count;

Small note: negative param count is a hack to avoid copying around. I'm
open for other suggestions. (let's discuss that later.)

—
Reply to this email directly or view it on GitHub
https://github.com/php/php-src/pull/1679/files#r47782276.

fci.object = fci_cache.object = newobj;
fci_cache.initialized = 1;
fci_cache.called_scope = Z_OBJCE_P(newthis);
Expand DownExpand Up@@ -277,6 +277,7 @@ ZEND_API zend_function *zend_get_closure_invoke_method(zend_object *object) /* {
invoke->internal_function.module = 0;
invoke->internal_function.scope = zend_ce_closure;
invoke->internal_function.function_name = zend_string_init(ZEND_INVOKE_FUNC_NAME, sizeof(ZEND_INVOKE_FUNC_NAME)-1, 0);
invoke->internal_function.stack_size = ZEND_CALL_FRAME_SLOT * sizeof(zval);
return invoke;
}
/* }}} */
Expand Down
Loading