Skip to content

Avoid generating exception check in tracing JIT for uncoerced scalar typed property writes #23925

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
ndossche wants to merge 1 commit into
base: master
Choose a base branch
from
+14 −1
Open

Avoid generating exception check in tracing JIT for uncoerced scalar typed property writes #23925

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 14 additions & 1 deletion Zend/Optimizer/zend_inference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5234,7 +5234,20 @@ ZEND_API bool zend_may_throw_ex(const zend_op *opline, const zend_ssa_op *ssa_op
zend_hash_find_ptr(&ce->properties_info, prop_name);
if (prop_info) {
if (ZEND_TYPE_IS_SET(prop_info->type)) {
return 1;
uint32_t type_mask = ZEND_TYPE_PURE_MASK(prop_info->type);

/* The assignment can't fail if the property only accepts a single scalar type
* and the value already has this type: the old value has no destructor.
* If the property holds a reference, all its type sources accept this type because they accept the current value. */
if ((prop_info->flags & (ZEND_ACC_READONLY|ZEND_ACC_PPP_SET_MASK))
|| ZEND_TYPE_IS_COMPLEX(prop_info->type)

@Girgias Girgias Sep 26, 2026

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.

Probably a dumb question, but why do we need to exclude single class types? Doesn't the SSA hold class information?

@ndossche ndossche Sep 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Complex types such as objects or arrays may invoke destructors. Or do you mean something else?

@Girgias Girgias Sep 26, 2026

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.

Right, and we still need the exception checks in those cases as when writing to the property the current value might be dropped.

That might be useful to clarify in a comment, but maybe this is just something one is mean to know when touching this code.

/* can't include bool due to references to false or true types. */

@ndossche ndossche Sep 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I realize now this comment is misleading: should be combined with the "single type" comment below. We can allow true and false as standalone types, but we can't allow bool due to references being shared with true/false standalone types.

|| (type_mask & ~(MAY_BE_NULL|MAY_BE_BOOL|MAY_BE_LONG|MAY_BE_DOUBLE|MAY_BE_STRING))
/* single type */
|| (type_mask & (type_mask - 1))
|| (OP1_DATA_INFO() & (MAY_BE_ANY|MAY_BE_UNDEF|MAY_BE_REF)) != type_mask) {
return 1;
}
}
return !(prop_info->flags & ZEND_ACC_PUBLIC)
&& prop_info->ce != op_array->scope;
Expand Down
Loading