Improve how the variadic placeholder is passed

 * We can't handle the variadic placeholder like non-variadic ones, as the slot
   may be occupied by a named arg.
 * Using Z_EXTRA(arg0) works, but it's uninitialized unless we set it in all
   SEND_PLACEHOLDER ops.

Author: arnaud-lb

Zend/tests/partial_application/pipe_optimization_008.phpt

@@ -26,7 +26,7 @@ try {
 ?>
 --EXPECTF--
 $_main:
-     ; (lines=19, args=0, vars=1, tmps=2)
+     ; (lines=18, args=0, vars=1, tmps=2)
      ; (after optimizer)
      ; %spipe_optimization_008.php:1-16
 0000 INIT_FCALL 0 %d string("time")
@@ -36,20 +36,19 @@ $_main:
 0004 DECLARE_FUNCTION string("foo") 0
 0005 INIT_FCALL_BY_NAME 0 string("foo")
 0006 SEND_VAL_EX int(1) string("a")
-0007 SEND_PLACEHOLDER
-0008 T1 = CALLABLE_CONVERT_PARTIAL %d
-0009 INIT_DYNAMIC_CALL 1 T1
-0010 SEND_VAL_EX int(2) 1
-0011 DO_FCALL
-0012 RETURN int(1)
-0013 CV0($e) = CATCH string("Throwable")
-0014 INIT_METHOD_CALL 0 CV0($e) string("getMessage")
-0015 V1 = DO_FCALL
-0016 ECHO V1
-0017 ECHO string("\n")
-0018 RETURN int(1)
+0007 T1 = CALLABLE_CONVERT_PARTIAL %d
+0008 INIT_DYNAMIC_CALL 1 T1
+0009 SEND_VAL_EX int(2) 1
+0010 DO_FCALL
+0011 RETURN int(1)
+0012 CV0($e) = CATCH string("Throwable")
+0013 INIT_METHOD_CALL 0 CV0($e) string("getMessage")
+0014 V1 = DO_FCALL
+0015 ECHO V1
+0016 ECHO string("\n")
+0017 RETURN int(1)
 EXCEPTION TABLE:
-     0005, 0013, -, -
+     0005, 0012, -, -
 
 foo:
      ; (lines=7, args=2, vars=2, tmps=0)

Zend/zend_compile.c

@@ -3714,12 +3714,13 @@ static uint32_t zend_get_arg_num(const zend_function *fn, const zend_string *arg
 
 static uint32_t zend_compile_args(
 		zend_ast *ast, const zend_function *fbc,
-		bool is_call_partial, bool *may_have_extra_named_args) /* {{{ */
+		bool is_call_partial, bool *may_have_extra_named_args,
+		bool *uses_variadic_placeholder_p) /* {{{ */
 {
 	const zend_ast_list *args = zend_ast_get_list(ast);
 	uint32_t i;
 	bool uses_arg_unpack = false;
-	bool uses_variadic_placeholder = 0;
+	bool uses_variadic_placeholder = false;
 	uint32_t arg_count = 0; /* number of arguments not including unpacks */
 
 	/* Whether named arguments are used syntactically, to enforce language level limitations.
@@ -3831,12 +3832,17 @@ static uint32_t zend_compile_args(
 					"Cannot combine partial application and unpacking");
 			}
 
+			// TODO: why?
+			fbc = NULL;
+
 			if (arg->attr == _IS_PLACEHOLDER_VARIADIC) {
 				uses_variadic_placeholder = true;
+				/* Do not emit ZEND_SEND_PLACEHOLDER: We represent the variadic
+				 * placeholder with a flag on the ZEND_CONVERT_CALLABLE_PARTIAL
+				 * op instead. */
+				continue;
 			}
 
-			fbc = NULL;
-
 			opline = zend_emit_op(NULL, ZEND_SEND_PLACEHOLDER, NULL, NULL);
 			opline->op1.num = arg->attr;
 			if (arg_name) {
@@ -3966,6 +3972,8 @@ static uint32_t zend_compile_args(
 		if (may_have_undef) {
 			zend_emit_op(NULL, ZEND_CHECK_UNDEF_ARGS, NULL, NULL);
 		}
+	} else {
+		*uses_variadic_placeholder_p = uses_variadic_placeholder;
 	}
 
 	return arg_count;
@@ -4006,7 +4014,8 @@ ZEND_API uint8_t zend_get_call_op(const zend_op *init_op, const zend_function *f
 /* }}} */
 
 void zend_compile_call_partial(znode *result, const zend_ast *call_ast,
-		uint32_t arg_count, bool may_have_extra_named_args, uint32_t opnum_init,
+		uint32_t arg_count, bool may_have_extra_named_args,
+		bool uses_variadic_placeholder, uint32_t opnum_init,
 		const zend_function *fbc) { /* {{{ */
 
 	zend_op *init_opline = &CG(active_op_array)->opcodes[opnum_init];
@@ -4027,6 +4036,9 @@ void zend_compile_call_partial(znode *result, const zend_ast *call_ast,
 	if (may_have_extra_named_args) {
 		opline->extended_value = ZEND_FCALL_MAY_HAVE_EXTRA_NAMED_PARAMS;
 	}
+	if (uses_variadic_placeholder) {
+		opline->extended_value |= ZEND_FCALL_USES_VARIADIC_PLACEHOLDER;
+	}
 
 	/* Collect placeholder order */
 	int level = 0;
@@ -4083,6 +4095,8 @@ static bool zend_compile_call_common(znode *result, const zend_ast *call_ast, ze
 	bool may_have_extra_named_args;
 
 	if (args_ast->kind == ZEND_AST_CALLABLE_CONVERT) {
+		bool uses_variadic_placeholder;
+
 		opline = &CG(active_op_array)->opcodes[opnum_init];
 		opline->extended_value = 0;
 		/* opcode array may be reallocated, so don't access opcode field after zend_emit_op_tmp(). */
@@ -4115,16 +4129,17 @@ static bool zend_compile_call_common(znode *result, const zend_ast *call_ast, ze
 
 		args_ast = ((zend_ast_fcc*)args_ast)->args;
 		uint32_t arg_count = zend_compile_args(args_ast, fbc, true,
-				&may_have_extra_named_args);
+				&may_have_extra_named_args, &uses_variadic_placeholder);
 
 		zend_compile_call_partial(result, call_ast, arg_count,
-				may_have_extra_named_args, opnum_init, fbc);
+				may_have_extra_named_args, uses_variadic_placeholder,
+				opnum_init, fbc);
 
 		return true;
 	}
 
 	uint32_t arg_count = zend_compile_args(args_ast, fbc,
-			false, &may_have_extra_named_args);
+			false, &may_have_extra_named_args, NULL);
 
 	zend_do_extended_fcall_begin();
 

Zend/zend_compile.h

@@ -1125,7 +1125,8 @@ ZEND_API zend_string *zend_type_to_string(zend_type type);
 
 #define ZEND_THROW_IS_EXPR 1u
 
-#define ZEND_FCALL_MAY_HAVE_EXTRA_NAMED_PARAMS 1
+#define ZEND_FCALL_MAY_HAVE_EXTRA_NAMED_PARAMS (1<<0)
+#define ZEND_FCALL_USES_VARIADIC_PLACEHOLDER   (1<<1)
 
 /* The send mode, the is_variadic, the is_promoted, and the is_tentative flags are stored as part of zend_type */
 #define _ZEND_SEND_MODE_SHIFT _ZEND_TYPE_EXTRA_FLAGS_SHIFT

Zend/zend_partial.c

@@ -78,7 +78,8 @@ ZEND_COLD static zend_never_inline void zend_partial_args_overflow(
 
 static zend_result zp_args_check(const zend_function *function,
 		uint32_t argc, const zval *argv,
-		const zend_array *extra_named_args) {
+		const zend_array *extra_named_args,
+		bool uses_variadic_placeholder) {
 
 	if (extra_named_args) {
 		zval *arg;
@@ -93,13 +94,11 @@ static zend_result zp_args_check(const zend_function *function,
 		} ZEND_HASH_FOREACH_END();
 	}
 
-	/* Z_EXTRA(ZEND_CALL_ARG(call, 1)) is set by ZEND_SEND_PLACEHOLDER */
-	bool variadic = Z_EXTRA(argv[0]) == _IS_PLACEHOLDER_VARIADIC;
-
-	uint32_t num = argc + (variadic ? -1 : 0);
+	// TODO: should be just 'argc', since the placeholder param doesn't increase argc?
+	uint32_t num = argc + (uses_variadic_placeholder ? -1 : 0);
 
 	if (num < function->common.required_num_args) {
-		if (variadic) {
+		if (uses_variadic_placeholder) {
 			return SUCCESS;
 		}
 
@@ -674,7 +673,8 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 		uint32_t argc, zval *argv, zend_array *extra_named_params,
 		const zend_array *named_positions,
 		const zend_op_array *declaring_op_array,
-		const zend_op *declaring_opline, void **cache_slot) {
+		const zend_op *declaring_opline, void **cache_slot,
+		bool uses_variadic_placeholder) {
 
 	zend_op_array *op_array = NULL;
 
@@ -684,7 +684,7 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 		return NULL;
 	}
 
-	if (UNEXPECTED(zp_args_check(function, argc, argv, extra_named_params) != SUCCESS)) {
+	if (UNEXPECTED(zp_args_check(function, argc, argv, extra_named_params, uses_variadic_placeholder) != SUCCESS)) {
 		ZEND_ASSERT(EG(exception));
 		return NULL;
 	}
@@ -702,16 +702,14 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 	int orig_lineno = CG(zend_lineno);
 	CG(zend_lineno) = zend_get_executed_lineno();
 
-	bool variadic_partial = false;
 	int new_argc = argc;
 
-	/* Z_EXTRA(ZEND_CALL_ARG(call, 1)) is set in ZEND_SEND_PLACEHOLDER */
-	if (Z_EXTRA(argv[0]) == _IS_PLACEHOLDER_VARIADIC) {
-		variadic_partial = true;
+	if (uses_variadic_placeholder) {
 		new_argc = MAX(new_argc, function->common.num_args);
 	}
 
 	zval *tmp = zend_arena_alloc(&CG(arena), new_argc * sizeof(zval));
+	// TODO: should be argc * sizeof(zval) ?
 	memcpy(tmp, argv, new_argc * sizeof(zval));
 	argv = tmp;
 
@@ -754,7 +752,7 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 					offset, param_offset, num_required);
 		}
 
-		if (variadic_partial) {
+		if (uses_variadic_placeholder) {
 			/* Handle implicit placeholders added by '...' */
 			for (uint32_t offset = 0; offset < new_argc; offset++) {
 				if (offset < argc && !Z_ISUNDEF(argv[offset])) {
@@ -780,13 +778,13 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 
 	/* Assign variable names */
 
-	uint32_t num_names = argc + variadic_partial + (extra_named_params != NULL)
+	uint32_t num_names = argc + uses_variadic_placeholder + (extra_named_params != NULL)
 		+ ((function->common.fn_flags & ZEND_ACC_CLOSURE) != 0);
 	zend_string **param_names = zend_arena_calloc(&CG(arena),
 			num_names, sizeof(zend_string*));
 	memset(param_names, 0, sizeof(zend_string*) * num_names);
 	zp_assign_names(param_names, num_names, argc, argv, function,
-			variadic_partial, extra_named_params);
+			uses_variadic_placeholder, extra_named_params);
 
 	/* Generate AST */
 
@@ -801,7 +799,7 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 		/* Must be first, as this is assumed in do_closure_bind() */
 		if (function->common.fn_flags & ZEND_ACC_CLOSURE) {
 			zend_ast *lexical_var_ast = zend_ast_create_zval_from_str(
-					zend_string_copy(param_names[argc + variadic_partial + (extra_named_params != NULL)]));
+					zend_string_copy(param_names[argc + uses_variadic_placeholder + (extra_named_params != NULL)]));
 			lexical_vars_ast = zend_ast_list_add(lexical_vars_ast, lexical_var_ast);
 		}
 
@@ -858,13 +856,13 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 
 	if (extra_named_params) {
 		zend_ast *lexical_var_ast = zend_ast_create_zval_from_str(
-				zend_string_copy(param_names[argc + variadic_partial]));
+				zend_string_copy(param_names[argc + uses_variadic_placeholder]));
 		lexical_vars_ast = zend_ast_list_add(lexical_vars_ast, lexical_var_ast);
 	}
 
 	/* If we have a variadic placeholder and the underlying function is
 	 * variadic, add a variadic param. */
-	if (variadic_partial
+	if (uses_variadic_placeholder
 			&& (function->common.fn_flags & ZEND_ACC_VARIADIC)) {
 		zend_arg_info *arg_info = &function->common.arg_info[function->common.num_args];
 		int param_flags = ZEND_PARAM_VARIADIC;
@@ -901,13 +899,13 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 	 * The func_num_args() call should be compiled to a single FUNC_NUM_ARGS op.
 	 */
 
-	if (variadic_partial && !(function->common.fn_flags & ZEND_ACC_VARIADIC)) {
+	if (uses_variadic_placeholder && !(function->common.fn_flags & ZEND_ACC_VARIADIC)) {
 		zend_ast *no_forwarding_ast = zend_ast_create_list(0, ZEND_AST_STMT_LIST);
 		zend_ast *forwarding_ast = zend_ast_create_list(0, ZEND_AST_STMT_LIST);
 
 		no_forwarding_ast = zp_compile_forwarding_call(this_ptr, function,
 				argc, argv, extra_named_params,
-				param_names, variadic_partial, num_params,
+				param_names, uses_variadic_placeholder, num_params,
 				called_scope, return_type, false, no_forwarding_ast);
 
 		if (!no_forwarding_ast) {
@@ -917,7 +915,7 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 
 		forwarding_ast = zp_compile_forwarding_call(this_ptr, function,
 				argc, argv, extra_named_params,
-				param_names, variadic_partial, num_params,
+				param_names, uses_variadic_placeholder, num_params,
 				called_scope, return_type, true, forwarding_ast);
 
 		if (!forwarding_ast) {
@@ -944,7 +942,7 @@ zend_op_array *zp_compile(zval *this_ptr, zend_function *function,
 	} else {
 		stmts_ast = zp_compile_forwarding_call(this_ptr, function,
 				argc, argv, extra_named_params,
-				param_names, variadic_partial, num_params,
+				param_names, uses_variadic_placeholder, num_params,
 				called_scope, return_type, false, stmts_ast);
 
 		if (!stmts_ast) {
@@ -1024,7 +1022,8 @@ zend_op_array *zp_get_op_array(zval *this_ptr, zend_function *function,
 		uint32_t argc, zval *argv, zend_array *extra_named_params,
 		const zend_array *named_positions,
 		const zend_op_array *declaring_op_array,
-		const zend_op *declaring_opline, void **cache_slot) {
+		const zend_op *declaring_opline, void **cache_slot,
+		bool uses_variadic_placeholder) {
 
 	if (EXPECTED(function->type == ZEND_INTERNAL_FUNCTION
 					? cache_slot[0] == function
@@ -1038,7 +1037,7 @@ zend_op_array *zp_get_op_array(zval *this_ptr, zend_function *function,
 	if (UNEXPECTED(!op_array)) {
 		op_array = zp_compile(this_ptr, function, argc, argv,
 			extra_named_params, named_positions, declaring_op_array, declaring_opline,
-			cache_slot);
+			cache_slot, uses_variadic_placeholder);
 	}
 
 	if (EXPECTED(op_array)) {
@@ -1103,12 +1102,13 @@ void zend_partial_create(zval *result, zval *this_ptr, zend_function *function,
 		uint32_t argc, zval *argv, zend_array *extra_named_params,
 		const zend_array *named_positions,
 		const zend_op_array *declaring_op_array,
-		const zend_op *declaring_opline, void **cache_slot) {
+		const zend_op *declaring_opline, void **cache_slot,
+		bool uses_variadic_placeholder) {
 
 	zend_op_array *op_array = zp_get_op_array(this_ptr, function, argc, argv,
 			extra_named_params, named_positions,
 			declaring_op_array, declaring_opline,
-			cache_slot);
+			cache_slot, uses_variadic_placeholder);
 
 	if (UNEXPECTED(!op_array)) {
 		ZEND_ASSERT(EG(exception));

Zend/zend_partial.h

@@ -27,7 +27,8 @@ void zend_partial_create(zval *result, zval *this_ptr, zend_function *function,
 		uint32_t argc, zval *argv, zend_array *extra_named_params,
 		const zend_array *named_positions,
 		const zend_op_array *declaring_op_array,
-		const zend_op *declaring_opline, void **cache_slot);
+		const zend_op *declaring_opline, void **cache_slot,
+		bool uses_variadic_placeholder);
 
 void zend_partial_op_array_dtor(zval *pDest);
 

Zend/zend_vm_def.h

@@ -9258,10 +9258,6 @@ ZEND_VM_HANDLER(211, ZEND_SEND_PLACEHOLDER, UNUSED, CONST|UNUSED)
 		ZEND_ASSERT(opline->op1.num == _IS_PLACEHOLDER_ARG);
 		Z_TYPE_INFO_P(arg) = _IS_PLACEHOLDER_ARG;
 		Z_EXTRA_P(ZEND_CALL_ARG(call, 1)) = 0;
-	} else if (opline->op1.num == _IS_PLACEHOLDER_VARIADIC) {
-		/* TODO: maybe find another way to pass the variadic flag, as this
-		 * requires to initialize this slot for each placeholder. */
-		Z_EXTRA_P(ZEND_CALL_ARG(call, 1)) = _IS_PLACEHOLDER_VARIADIC;
 	} else {
 		ZEND_ASSERT(opline->op1.num == _IS_PLACEHOLDER_ARG);
 		arg = ZEND_CALL_VAR(EX(call), opline->result.var);
@@ -9794,6 +9790,7 @@ ZEND_VM_HANDLER(202, ZEND_CALLABLE_CONVERT, UNUSED, UNUSED, NUM|CACHE_SLOT)
 	ZEND_VM_NEXT_OPCODE();
 }
 
+// TODO: verify handler flags. We have a cache slot on op1, some flags on extended_value
 ZEND_VM_HANDLER(212, ZEND_CALLABLE_CONVERT_PARTIAL, NUM, CONST|UNUSED, CACHE_SLOT)
 {
 	USE_OPLINE
@@ -9809,7 +9806,8 @@ ZEND_VM_HANDLER(212, ZEND_CALLABLE_CONVERT_PARTIAL, NUM, CONST|UNUSED, CACHE_SLO
 		ZEND_CALL_INFO(call) & ZEND_CALL_HAS_EXTRA_NAMED_PARAMS ?
 			call->extra_named_params : NULL,
 		OP2_TYPE == IS_CONST ? Z_ARRVAL_P(named_positions) : NULL,
-		&EX(func)->op_array, opline, cache_slot);
+		&EX(func)->op_array, opline, cache_slot,
+		opline->extended_value & ZEND_FCALL_USES_VARIADIC_PLACEHOLDER);
 
 	if (ZEND_CALL_INFO(call) & ZEND_CALL_HAS_EXTRA_NAMED_PARAMS) {
 		zend_array_release(call->extra_named_params);

ext/opcache/jit/zend_jit_ir.c

@@ -10078,7 +10078,7 @@ static int zend_jit_do_fcall(zend_jit_ctx *jit, const zend_op *opline, const zen
 	}
 
 	bool may_have_extra_named_params =
-		opline->extended_value == ZEND_FCALL_MAY_HAVE_EXTRA_NAMED_PARAMS &&
+		(opline->extended_value & ZEND_FCALL_MAY_HAVE_EXTRA_NAMED_PARAMS) &&
 		(!func || func->common.fn_flags & ZEND_ACC_VARIADIC);
 
 	if (!jit->reuse_ip) {