Skip to content

FFI::CType reflection API #7217

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
wants to merge 5 commits into from
Closed

FFI::CType reflection API #7217

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b040e24
FFI::CType reflection API
dstogov Jul 6, 2021
070c48f
Fixed refcounting
dstogov Jul 6, 2021
e8265f1
Fixed return type namespaces
dstogov Jul 6, 2021
779ff7b
Follow to Nikita's method naming suggestions,
dstogov Jul 6, 2021
c580ae5
Use better argument names
dstogov Jul 7, 2021
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
358 changes: 358 additions & 0 deletions ext/ffi/ffi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4469,6 +4469,318 @@ ZEND_METHOD(FFI_CType, getName) /* {{{ */
RETURN_STR(res);
}
}
/* }}} */

ZEND_METHOD(FFI_CType, getKind) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
RETURN_LONG(type->kind);
}
/* }}} */

ZEND_METHOD(FFI_CType, getSize) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
RETURN_LONG(type->size);
}
/* }}} */

ZEND_METHOD(FFI_CType, getAlignment) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
RETURN_LONG(type->align);
}
/* }}} */

ZEND_METHOD(FFI_CType, getAttributes) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
RETURN_LONG(type->attr);
}
/* }}} */

ZEND_METHOD(FFI_CType, getEnumKind) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}
Comment on lines +4535 to +4537
Copy link
Contributor

@TysonAndre TysonAndre Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}
ZEND_PARSE_PARAMETERS_NONE();

nit: Could be shortened here and elsewhere. Is there a reason for different parts of the codebase using different ways of writing this?


type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_ENUM) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not an enumeration");
RETURN_THROWS();
}
RETURN_LONG(type->enumeration.kind);
}
/* }}} */

ZEND_METHOD(FFI_CType, getArrayElementType) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about extraction of separate CArrayType for this? It can make API stable and more logical, as there is no point in calling getArrayElementType for simple scalar types - it will always throw an error, which can be example of LSP principle - we will have methods that won't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general, it's possible to introduce and instantiate CArrayType instead of CType, but I don't like to do this in last minute and without RFC.

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;
zend_ffi_ctype *ret;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_ARRAY) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not an array");
Copy link
Contributor

Choose a reason for hiding this comment

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

This runtime check can be avoid by moving implementation into the separate CArrayType

RETURN_THROWS();
}

ret = (zend_ffi_ctype*)zend_ffi_ctype_new(zend_ffi_ctype_ce);
ret->type = ZEND_FFI_TYPE(type->array.type);
RETURN_OBJ(&ret->std);
}
/* }}} */

ZEND_METHOD(FFI_CType, getArrayLength) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

Would implementing Countable and calling this Countable::count() make sense here (continuing to throw for non-arrays)? for ($i = 0; $i < count($ffiArray); $i++) {...}

Then again, that'd be a much larger change and that interface isn't in FFI\CData for 8.0

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_ARRAY) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not an array");
RETURN_THROWS();
}
RETURN_LONG(type->array.length);
}
/* }}} */

ZEND_METHOD(FFI_CType, getPointerType) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we can have separate CPointerType, which will give us better visibility in PHP code.

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_ctype *ret;
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_POINTER) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a pointer");
RETURN_THROWS();
}

ret = (zend_ffi_ctype*)zend_ffi_ctype_new(zend_ffi_ctype_ce);
ret->type = ZEND_FFI_TYPE(type->pointer.type);
RETURN_OBJ(&ret->std);
}
/* }}} */

ZEND_METHOD(FFI_CType, getStructFieldNames) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;
HashTable *ht;
zend_string* name;
zval zv;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_STRUCT) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a structure");
RETURN_THROWS();
}

ht = zend_new_array(zend_hash_num_elements(&type->record.fields));
RETVAL_ARR(ht);
ZEND_HASH_FOREACH_STR_KEY(&type->record.fields, name) {
ZVAL_STR_COPY(&zv, name);
zend_hash_next_index_insert_new(ht, &zv);
} ZEND_HASH_FOREACH_END();
}
/* }}} */

ZEND_METHOD(FFI_CType, getStructFieldOffset) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

getStructureFieldOffset() ?

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;
zend_string *name;
zend_ffi_field *ptr;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(name)
ZEND_PARSE_PARAMETERS_END();

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_STRUCT) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a structure");
RETURN_THROWS();
}

ptr = zend_hash_find_ptr(&type->record.fields, name);
if (!ptr) {
zend_throw_error(zend_ffi_exception_ce, "Wrong fileld name");
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding field name into exception message to make it more obvious?

Copy link
Member

Choose a reason for hiding this comment

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

It also has a typo in the message

RETURN_THROWS();
}
RETURN_LONG(ptr->offset);
}
/* }}} */

ZEND_METHOD(FFI_CType, getStructFieldType) /* {{{ */
{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;
zend_string *name;
zend_ffi_field *ptr;
zend_ffi_ctype *ret;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(name)
ZEND_PARSE_PARAMETERS_END();

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_STRUCT) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a structure");
RETURN_THROWS();
}

ptr = zend_hash_find_ptr(&type->record.fields, name);
if (!ptr) {
zend_throw_error(zend_ffi_exception_ce, "Wrong fileld name");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add information about field name into exception too? To make it discoverable?

RETURN_THROWS();
}

ret = (zend_ffi_ctype*)zend_ffi_ctype_new(zend_ffi_ctype_ce);
ret->type = ZEND_FFI_TYPE(ptr->type);
RETURN_OBJ(&ret->std);
}
/* }}} */

ZEND_METHOD(FFI_CType, getFuncABI) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

getFunctionCallType()?
ABI loos very cryptic for non-experienced developers + better to use full words inside method names

Copy link
Member

Choose a reason for hiding this comment

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

I think keeping ABI is better here, because it's a term of art. If you really want to use a different term CallingConvention could work as well, but CallType doesn't seem correct in this context.

Copy link
Contributor

@lisachenko lisachenko Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, let's keep getFunctionABI() then if you don't have objections. I also like getFunctionCallingConvention() - this will be readable.

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_FUNC) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a function");
RETURN_THROWS();
}
RETURN_LONG(type->func.abi);
}
/* }}} */

ZEND_METHOD(FFI_CType, getFuncReturnType) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

getFunctionReturnType() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use the same names that reflection already uses for these methods? https://www.php.net/manual/en/reflectionfunctionabstract.getreturntype.php E.g. getReturnType

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_ctype *ret;
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_FUNC) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a function");
RETURN_THROWS();
}

ret = (zend_ffi_ctype*)zend_ffi_ctype_new(zend_ffi_ctype_ce);
ret->type = ZEND_FFI_TYPE(type->func.ret_type);
RETURN_OBJ(&ret->std);
}
/* }}} */

ZEND_METHOD(FFI_CType, getFuncArgCount) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

getFunctionArgumentCount()?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.php.net/manual/en/reflectionfunctionabstract.getnumberofparameters.php

Alternately, getNumberOfParameters

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type;

if (zend_parse_parameters_none() == FAILURE) {
RETURN_THROWS();
}

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_FUNC) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a function");
RETURN_THROWS();
}
RETURN_LONG(type->func.args ? zend_hash_num_elements(type->func.args) : 0);
}
/* }}} */

ZEND_METHOD(FFI_CType, getFuncArgType) /* {{{ */
Copy link
Contributor

Choose a reason for hiding this comment

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

getFunctionArgumentType()?

Copy link
Member Author

Choose a reason for hiding this comment

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

in my opinion, this longer name doesn't make things more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

PHP is already already using "Parameter" instead of "Argument" for reflection on types elsewhere.

Also, would omitting Func for something like getParameterType make more sense - nothing other than functions would have parameters.

{
zend_ffi_ctype *ctype = (zend_ffi_ctype*)(Z_OBJ_P(ZEND_THIS));
zend_ffi_type *type, *ptr;
zend_long n;
zend_ffi_ctype *ret;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_LONG(n)
Copy link
Contributor

Choose a reason for hiding this comment

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

argumentIndex?

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes sense, but not in this place but in stub, and probably $arg_index.

ZEND_PARSE_PARAMETERS_END();

type = ZEND_FFI_TYPE(ctype->type);
if (type->kind != ZEND_FFI_TYPE_FUNC) {
zend_throw_error(zend_ffi_exception_ce, "FFI\\CType is not a function");
RETURN_THROWS();
}

if (!type->func.args) {
zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe state explicitly that "Function $name has not arguments".

Copy link
Member Author

Choose a reason for hiding this comment

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

May be this makes sense, however, we don't know the function name at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then current one should be ok

Copy link
Contributor

@TysonAndre TysonAndre Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");
zend_throw_error(zend_ffi_exception_ce, "Missing parameter offset #d of FFI function with %d parameter(s)", ...);

Maybe something more along these lines with the argument count and offset being fetched, to speed up debugging? (my wording can be improved)

RETURN_THROWS();
}

ptr = zend_hash_index_find_ptr(type->func.args, n);
if (!ptr) {
zend_throw_error(zend_ffi_exception_ce, "Wrong argument number");
RETURN_THROWS();
}

ret = (zend_ffi_ctype*)zend_ffi_ctype_new(zend_ffi_ctype_ce);
ret->type = ZEND_FFI_TYPE(ptr);
RETURN_OBJ(&ret->std);
}
/* }}} */

static char *zend_ffi_parse_directives(const char *filename, char *code_pos, char **scope_name, char **lib, bool preload) /* {{{ */
{
Expand Down Expand Up @@ -4896,6 +5208,9 @@ static int zend_ffi_preload(char *preload) /* {{{ */
}
/* }}} */

#define REGISTER_FFI_TYPE_CONSTANT(name) \
zend_declare_class_constant_long(zend_ffi_ctype_ce, #name, sizeof(#name) - 1, ZEND_FFI_ ## name)

/* {{{ ZEND_MINIT_FUNCTION */
ZEND_MINIT_FUNCTION(ffi)
{
Expand Down Expand Up @@ -5046,6 +5361,49 @@ ZEND_MINIT_FUNCTION(ffi)
zend_ffi_ctype_handlers.get_properties = zend_fake_get_properties;
zend_ffi_ctype_handlers.get_gc = zend_fake_get_gc;

REGISTER_FFI_TYPE_CONSTANT(TYPE_VOID);
REGISTER_FFI_TYPE_CONSTANT(TYPE_FLOAT);
REGISTER_FFI_TYPE_CONSTANT(TYPE_DOUBLE);
#ifdef HAVE_LONG_DOUBLE
REGISTER_FFI_TYPE_CONSTANT(TYPE_LONGDOUBLE);
#endif
REGISTER_FFI_TYPE_CONSTANT(TYPE_UINT8);
REGISTER_FFI_TYPE_CONSTANT(TYPE_SINT8);
REGISTER_FFI_TYPE_CONSTANT(TYPE_UINT16);
REGISTER_FFI_TYPE_CONSTANT(TYPE_SINT16);
REGISTER_FFI_TYPE_CONSTANT(TYPE_UINT32);
REGISTER_FFI_TYPE_CONSTANT(TYPE_SINT32);
REGISTER_FFI_TYPE_CONSTANT(TYPE_UINT64);
REGISTER_FFI_TYPE_CONSTANT(TYPE_SINT64);
REGISTER_FFI_TYPE_CONSTANT(TYPE_ENUM);
REGISTER_FFI_TYPE_CONSTANT(TYPE_BOOL);
REGISTER_FFI_TYPE_CONSTANT(TYPE_CHAR);
REGISTER_FFI_TYPE_CONSTANT(TYPE_POINTER);
REGISTER_FFI_TYPE_CONSTANT(TYPE_FUNC);
REGISTER_FFI_TYPE_CONSTANT(TYPE_ARRAY);
REGISTER_FFI_TYPE_CONSTANT(TYPE_STRUCT);

REGISTER_FFI_TYPE_CONSTANT(ATTR_CONST);
REGISTER_FFI_TYPE_CONSTANT(ATTR_INCOMPLETE_TAG);
REGISTER_FFI_TYPE_CONSTANT(ATTR_VARIADIC);
REGISTER_FFI_TYPE_CONSTANT(ATTR_INCOMPLETE_ARRAY);
REGISTER_FFI_TYPE_CONSTANT(ATTR_VLA);
REGISTER_FFI_TYPE_CONSTANT(ATTR_UNION);
REGISTER_FFI_TYPE_CONSTANT(ATTR_PACKED);
REGISTER_FFI_TYPE_CONSTANT(ATTR_MS_STRUCT);
REGISTER_FFI_TYPE_CONSTANT(ATTR_GCC_STRUCT);

REGISTER_FFI_TYPE_CONSTANT(ABI_DEFAULT);
REGISTER_FFI_TYPE_CONSTANT(ABI_CDECL);
REGISTER_FFI_TYPE_CONSTANT(ABI_FASTCALL);
REGISTER_FFI_TYPE_CONSTANT(ABI_THISCALL);
REGISTER_FFI_TYPE_CONSTANT(ABI_STDCALL);
REGISTER_FFI_TYPE_CONSTANT(ABI_PASCAL);
REGISTER_FFI_TYPE_CONSTANT(ABI_REGISTER);
REGISTER_FFI_TYPE_CONSTANT(ABI_MS);
REGISTER_FFI_TYPE_CONSTANT(ABI_SYSV);
REGISTER_FFI_TYPE_CONSTANT(ABI_VECTORCALL);

if (FFI_G(preload)) {
if (zend_ffi_preload(FFI_G(preload)) != SUCCESS) {
return FAILURE;
Expand Down
Loading