Skip to content

Use zend_ast_size consistenly #11955

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.

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

Merged
merged 3 commits into from
Aug 13, 2023

Conversation

crrodriguez
Copy link
Contributor

Ths calculations are error prone and subject to change.
THis also fixes zend_ast_size implementation to align with other simlar wrappers.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Looks ok to me otherwise. The XtOffsetOf is not truly less error prone, as we're still relying on the fact that child is the last element. In theory this removes padding of zend_ast from the size, in practice I don't know when there would be padding, or whether that's relevant.

@crrodriguez
Copy link
Contributor Author

Looks ok to me otherwise. The XtOffsetOf is not truly less error prone, as we're still relying on the fact that child is the last element. In theory this removes padding of zend_ast from the size, in practice I don't know when there would be padding, or whether that's relevant.

child must be the last element, if it is implemented with either the struct hack, zero length extension or C99 flex array.. using offsetof it will return the same value even if I change the struct definition to use proper c99 flexible arrays instead (this patchset is part of initial work to make using c99 flexible arrays instead of the "struck hack" easier)

It is better not to use sizeof(struct_with_flexible_array)
and instead rely on offsetof(type, member) like most
other similar wrappers do.
@crrodriguez crrodriguez force-pushed the zend_ast_size_collateral_evo branch from 7248014 to f06c283 Compare August 13, 2023 14:42
@iluuu1994
Copy link
Member

@crrodriguez What I meant is that with the struct hack, adding a new element after child would not result in an error (apart from out-of-bounds warnings, probably), so it's not safer atm. Anyway, this this would fail all over the place anyway, so is not really relevant. If the plan is to move to flexible arrays at some point then the change makes sense of course.

@iluuu1994 iluuu1994 merged commit 2196e22 into php:master Aug 13, 2023
@iluuu1994
Copy link
Member

Thank you @crrodriguez!

@crrodriguez crrodriguez deleted the zend_ast_size_collateral_evo branch August 14, 2023 02:11
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
* opcache: use zend_ast_size helper in zend_persist_ast

* opcache: use zend_ast_size helper in zend_persist_ast_calc

* Zend: fix zend_ast_size definition

It is better not to use sizeof(struct_with_flexible_array)
and instead rely on offsetof(type, member) like most
other similar wrappers do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants