Simplify Callable implementation

[sjinks]

This PR addresses the following comment found in the code of zend/callable.cpp:

 /**
 *  Map function names to their implementation
 *
 *  This is braindead, there should be a way to get this information
 *  from the "This" zval in the execute_data, I just can't find it
 *  @todo   Find a better way for this
 *  @var    std::map<std::string, Callable*>
 */
static std::map<std::string, Callable*> callables;

Though the required information cannot be found in EG(This) (especially for function calls — functions have no $this), yet still there is a way to store and retrieve that information in Zend structures.

The idea is that we allocate two arginfo's more than necessary:

-        _argv(new zend_internal_arg_info[_argc + 1])
+        _argv(new zend_internal_arg_info[_argc + 3])

The first extra arginfo will be a sentinel with its name and class_name set to nullptr, the second one will store this of the Callable itself in class_name (and name will be nullptr).

If necessary, we can live without the sentinel and save some memory (12 bytes for 32 bit, 24 bytes for 64 bit), it is used only to simplify debugging.

The initialization becomes as simple as that:

_argv[_argc + 2].class_name = reinterpret_cast<const char*>(this);

Retrieval of the Callable:

uint32_t argc       = EX(func)->common.num_args;
zend_arg_info* info = EX(func)->common.arg_info;

// Sanity check
assert(info[argc+1].class_name == nullptr && info[argc+1].name == nullptr);
assert(info[argc+2].class_name != nullptr && info[argc+2].name == nullptr);

Callable *callable = reinterpret_cast<Callable*>(info[argc+2].class_name);

Bonus: since this code was asking for trouble:

    Callable(const Callable &that) :
        _name(that._name),
        _return(that._return),
        _required(that._required),
        _argc(that._argc),
        _argv(nullptr) {}
        // @todo: we have no arguments after copy? is this correct?
        // we do have the argument count though...

I have marked that copy constructor and other copy constructors depending on it as deleted. It looks like copying of callables was not used by the library…

This modified implementation successfully passes all tests (BTW, if you are interested in setting up Travis CI, I have tests to share 😉 )

[sjinks]

Pretty interesting: that code is not invoked…

https://codecov.io/gh/sjinks/PHP-CPP/src/tests/zend/callable.cpp#L25

[ghost]

It can actually be called, but only if you use a deprecated way of defining your member functions. If you look in include/class.h you will find a number of overloads for the add method that are deprecated. They give the callback as a regular parameter instead of a template parameter. These methods should use the code you have improved.

The whole point of this was to eventually be able to remove the old methods (because they are so inefficient) - but until we do that it won't hurt to optimize for this. Can you see if your new code works properly with this? If so I'd be happy to merge it.

About setting up Travis CI, this is also something I'm interested in, but haven't done due to lack of time and experience in this field. We used to have tests for PHP-CPP, but they were removed by someone when they had grown outdated and people were asking questions. If you can help me set up automated tests I think it would be a huge improvement.

[ghost]

I just tested your code, and this approach unfortunately doesn't work. As you can see in callable.h, we create an array of zend_internal_arg_info structures. You can never actually retrieve them anywhere from PHP. They get copied to an array of zend_arg_info structures which look alike but aren't.

The extra entries are not copied, so we end up with undefined behavior.

[sjinks]

@martijnotto The code does fail but for another reason.

In fact, arginfo is never copied. lxr.php.net is down, so I will have to paste the code here:

zend_register_functions() in Zend/zend_API.c, line 2177 in PHP 7.0.9:

if (ptr->arg_info) {
	zend_internal_function_info *info = (zend_internal_function_info*)ptr->arg_info;

	internal_function->arg_info = (zend_internal_arg_info*)ptr->arg_info+1;
	internal_function->num_args = ptr->num_args;
	/* Currently you cannot denote that the function can accept less arguments than num_args */
	if (info->required_num_args == (zend_uintptr_t)-1) {
		internal_function->required_num_args = ptr->num_args;
	} else {
		internal_function->required_num_args = info->required_num_args;
	}
	if (info->return_reference) {
		internal_function->fn_flags |= ZEND_ACC_RETURN_REFERENCE;
	}
	if (ptr->arg_info[ptr->num_args].is_variadic) {
		// ...
	}
	if (info->type_hint) {
		// ...
	}
} else {
	internal_function->arg_info = NULL;
	internal_function->num_args = 0;
	internal_function->required_num_args = 0;
}

The line of interest is this:

internal_function->arg_info = (zend_internal_arg_info*)ptr->arg_info+1;

function's argument information is just a pointer to the originally provided arginfo array plus one element (zeroth one contains information about the function itself).

zend_internal_arg_info and zend_arg_info have identical layout and differ only in pointer types (const char* vs zend_string*), but since the pointer size is the same, this does not matter.

/* arg_info for internal functions */
typedef struct _zend_internal_arg_info {
	const char *name;
	const char *class_name;
	zend_uchar type_hint;
	zend_uchar pass_by_reference;
	zend_bool allow_null;
	zend_bool is_variadic;
} zend_internal_arg_info;

/* arg_info for user functions */
typedef struct _zend_arg_info {
	zend_string *name;
	zend_string *class_name;
	zend_uchar type_hint;
	zend_uchar pass_by_reference;
	zend_bool allow_null;
	zend_bool is_variadic;
} zend_arg_info;

The issue is that I was off by one in Callable::invoke because of that very ptr->arg_info+1:

     // Sanity check
-    assert(info[argc+1].class_name == nullptr && info[argc+1].name == nullptr);
-    assert(info[argc+2].class_name != nullptr && info[argc+2].name == nullptr);
+    assert(info[argc+0].class_name == nullptr && info[argc+0].name == nullptr);
+    assert(info[argc+1].class_name != nullptr && info[argc+1].name == nullptr);

     // the callable we are retrieving
-    Callable *callable = reinterpret_cast<Callable*>(info[argc+2].class_name);
+    Callable *callable = reinterpret_cast<Callable*>(info[argc+1].class_name);

If you use the corrected implementation of invoke(), it should work.

Tested with:

#include <ostream>

static void test303()
{
    Php::out << "Hello world!" << std::endl;
}

// ...
e.add("test303", &test303);

--TEST--
Issue 303
--FILE--
<?php
test303();
?>
--EXPECT--
Hello world!

[sjinks]

The updated code is here: https://github.com/sjinks/PHP-CPP/tree/callable

[ghost]

Ah, that does solve the issues. I have merged it manually now, since I was unable to reopen the pull request. I have also removed the additional empty element. It seems to work just fine without this.