Skip to content

Added fallthrough for == operator with objects for extensions #7973

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added fallthrough for == operator with objects for extensions #7973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
348feb7
Added fallthrough for == operator with objects for extensions
JordanRL Jan 20, 2022
2fa6155
Added fallthrough for == operator with objects for extensions
JordanRL Jan 20, 2022
2be2077
Fixed looping bug in zend_equals_object
JordanRL Jan 20, 2022
de984eb
Merge branch 'php:master' into equals-fallthrough-vm
JordanRL May 13, 2022
c6440fa
Switching to a new handler based implementation
JordanRL May 14, 2022
8a2b6cd
Fixing broken compile
JordanRL May 14, 2022
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
3 changes: 2 additions & 1 deletion Zend/zend_iterators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ static const zend_object_handlers iterator_object_handlers = {
iter_wrapper_get_gc,
NULL, /* do_operation */
NULL, /* compare */
NULL /* get_properties_for */
NULL, /* get_properties_for */
NULL /* equals */
};

ZEND_API void zend_register_iterator_wrapper(void)
Expand Down
5 changes: 5 additions & 0 deletions Zend/zend_object_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1941,6 +1941,10 @@ ZEND_API HashTable *zend_get_properties_for(zval *obj, zend_prop_purpose purpose
return zend_std_get_properties_for(zobj, purpose);
}

ZEND_API zend_result zend_std_equals_objects(zend_uchar opcode, zval *result, zval *op1, zval *op2) {
return FAILURE;
Copy link
Member

Choose a reason for hiding this comment

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

Can the fallback to the compare handler be in here?

Do we really need to pass the opcode here? I think it would be better for the return value to indicate whether it equals or not (possibly with an equals/not_equals/uncomparable enum). This would prevent inconsistent == and != implementations by design.

Copy link
Author

Choose a reason for hiding this comment

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

The reason I didn't put the fallback here is that extensions that used this handler would have to manually implement the fallback again (assuming they wanted to use the compare handler). But yeah, if this function returned a result enum instead of zend_result I think the fallback would have to go in here.

That's just very different from how the rest of the handlers are done (from what I've seen), and I wanted this code to be similar to other code in the engine.

Copy link
Member

Choose a reason for hiding this comment

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

The reason I didn't put the fallback here is that extensions that used this handler would have to manually implement the fallback again (assuming they wanted to use the compare handler).

Not sure I get what you mean here -- if they want to use some custom logic and then fall back to the default, they can call zend_std_equals_objects to do so.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure I get what you mean here -- if they want to use some custom logic and then fall back to the default, they can call zend_std_equals_objects to do so.

Or yeah... they could do the obvious thing you just suggested...

Copy link
Author

Choose a reason for hiding this comment

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

Actually though, what about the case where op1 falls back but op2 has custom logic? How would the default handler for op1 handle that?

}

ZEND_API const zend_object_handlers std_object_handlers = {
0, /* offset */

Expand Down Expand Up @@ -1969,4 +1973,5 @@ ZEND_API const zend_object_handlers std_object_handlers = {
NULL, /* do_operation */
zend_std_compare_objects, /* compare */
NULL, /* get_properties_for */
zend_std_equals_objects, /* equals */
};
3 changes: 3 additions & 0 deletions Zend/zend_object_handlers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ typedef HashTable *(*zend_object_get_gc_t)(zend_object *object, zval **table, in

typedef int (*zend_object_do_operation_t)(zend_uchar opcode, zval *result, zval *op1, zval *op2);

typedef zend_result (*zend_object_equals_t)(zend_uchar opcode, zval *result, zval *op1, zval *op2);

struct _zend_object_handlers {
/* offset of real object header (usually zero) */
int offset;
Expand Down Expand Up @@ -184,6 +186,7 @@ struct _zend_object_handlers {
zend_object_do_operation_t do_operation; /* optional */
zend_object_compare_t compare; /* required */
zend_object_get_properties_for_t get_properties_for; /* optional */
zend_object_equals_t equals; /* optional */
};

BEGIN_EXTERN_C()
Expand Down
49 changes: 48 additions & 1 deletion Zend/zend_operators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ ZEND_API const unsigned char zend_toupper_map[256] = {
0xc0,0xc1,0xc2,0xc3,0xc4,0xc5,0xc6,0xc7,0xc8,0xc9,0xca,0xcb,0xcc,0xcd,0xce,0xcf,
0xd0,0xd1,0xd2,0xd3,0xd4,0xd5,0xd6,0xd7,0xd8,0xd9,0xda,0xdb,0xdc,0xdd,0xde,0xdf,
0xe0,0xe1,0xe2,0xe3,0xe4,0xe5,0xe6,0xe7,0xe8,0xe9,0xea,0xeb,0xec,0xed,0xee,0xef,
0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,0xf7,0xf8,0xf9,0xfa,0xfb,0xfc,0xfd,0xfe,0xff
0xf0,0xf1,0xf2,0xf3,0xf4,0xf5,0xf6,0xf7,0xf8,0xf9,0xfa,0xfb,0xfc,0xfd,0xfe,0xff
};


Expand Down Expand Up @@ -2132,6 +2132,53 @@ static int compare_double_to_string(double dval, zend_string *str) /* {{{ */
}
/* }}} */

ZEND_API int ZEND_FASTCALL zend_equals_object(zval *op1, zval *op2, zend_uchar equals) /* {{{ */
{
zval ret;
int retVal;
int result;
if (Z_TYPE_P(op1) == IS_OBJECT &&
Z_TYPE_P(op2) == IS_OBJECT &&
Z_OBJ_P(op1) == Z_OBJ_P(op2)) {
return 0;
} else if (Z_TYPE_P(op1) == IS_OBJECT) {
if (Z_OBJ_HANDLER_P(op1, equals)) {
result = Z_OBJ_HANDLER_P(op1, equals)(equals, &ret, op1, op2);
if (result == FAILURE) {
goto op2_equals_handler;
} else {
retVal = (Z_TYPE_INFO(ret) == IS_TRUE ? 0 : ZEND_UNCOMPARABLE);
}
return retVal;
} else {
if (Z_TYPE_P(op2) == IS_OBJECT) {
goto op2_equals_handler;
} else {
goto default_equals_handler;
}
}
} else if (Z_TYPE_P(op2) == IS_OBJECT) {
op2_equals_handler:
if (Z_OBJ_HANDLER_P(op2, equals)) {
result = Z_OBJ_HANDLER_P(op2, equals)(equals, &ret, op1, op2);
if (result == FAILURE) {
goto default_equals_handler;
} else {
retVal = (Z_TYPE_INFO(ret) == IS_TRUE ? 0 : ZEND_UNCOMPARABLE);
}
return retVal;
} else {
goto default_equals_handler;
}
} else {
default_equals_handler:
return zend_compare(op1, op2);
}
}
/* }}} */



ZEND_API int ZEND_FASTCALL zend_compare(zval *op1, zval *op2) /* {{{ */
{
int converted = 0;
Expand Down
1 change: 1 addition & 0 deletions Zend/zend_operators.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,7 @@ static zend_always_inline bool i_zend_is_true(zval *op)
// TODO: Use a different value to allow an actual distinction here.
#define ZEND_UNCOMPARABLE 1

ZEND_API int ZEND_FASTCALL zend_equals_object(zval *op1, zval *op2, zend_uchar equals);
ZEND_API int ZEND_FASTCALL zend_compare(zval *op1, zval *op2);

ZEND_API zend_result ZEND_FASTCALL compare_function(zval *result, zval *op1, zval *op2);
Expand Down
12 changes: 10 additions & 2 deletions Zend/zend_vm_def.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,11 @@ ZEND_VM_HELPER(zend_is_equal_helper, ANY, ANY, zval *op_1, zval *op_2)
if (UNEXPECTED(Z_TYPE_INFO_P(op_2) == IS_UNDEF)) {
op_2 = ZVAL_UNDEFINED_OP2();
}
ret = zend_compare(op_1, op_2);
if (Z_TYPE_P(op_1) == IS_OBJECT || Z_TYPE_P(op_2) == IS_OBJECT) {
ret = zend_equals_object(op_1, op_2, ZEND_IS_EQUAL);
Copy link
Member

Choose a reason for hiding this comment

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

I believe you need a separate zend_equals that is pushed through the entire hierarchy. Otherwise objects nested in arrays will not use the correct handler, for example.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, similar to zend_compare that handles all types and type combinations? I hadn't considered the case of objects in arrays.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, similar to zend_compare that handles all types and type combinations?

Yeah, exactly.

} else {
ret = zend_compare(op_1, op_2);
}
if (OP1_TYPE & (IS_TMP_VAR|IS_VAR)) {
zval_ptr_dtor_nogc(op_1);
}
Expand Down Expand Up @@ -581,7 +585,11 @@ ZEND_VM_HELPER(zend_is_not_equal_helper, ANY, ANY, zval *op_1, zval *op_2)
if (UNEXPECTED(Z_TYPE_INFO_P(op_2) == IS_UNDEF)) {
op_2 = ZVAL_UNDEFINED_OP2();
}
ret = zend_compare(op_1, op_2);
if (Z_TYPE_P(op_1) == IS_OBJECT || Z_TYPE_P(op_2) == IS_OBJECT) {
ret = zend_equals_object(op_1, op_2, ZEND_IS_NOT_EQUAL);
} else {
ret = zend_compare(op_1, op_2);
}
if (OP1_TYPE & (IS_TMP_VAR|IS_VAR)) {
zval_ptr_dtor_nogc(op_1);
}
Expand Down
1 change: 1 addition & 0 deletions ext/com_dotnet/com_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,7 @@ zend_object_handlers php_com_object_handlers = {
NULL, /* do_operation */
com_objects_compare, /* compare */
NULL, /* get_properties_for */
NULL, /* equals */
};

void php_com_object_enable_event_sink(php_com_dotnet_object *obj, bool enable)
Expand Down
1 change: 1 addition & 0 deletions ext/com_dotnet/com_saproxy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ zend_object_handlers php_com_saproxy_handlers = {
NULL, /* do_operation */
saproxy_objects_compare, /* compare */
NULL, /* get_properties_for */
NULL, /* equals */
};

void php_com_saproxy_create(zend_object *com_object, zval *proxy_out, zval *index)
Expand Down