Skip to content

Assert ptr_ptr value of TMP|CONST isn't used #11865

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 1 commit into from
Aug 3, 2023

Conversation

iluuu1994
Copy link
Member

We require valid code for compilation to succeed, but these paths should always be guarded by OPx_TYPE checks and never execute. Add an assertion to verify.

Motivated by the comment here: #11788 (comment)

@iluuu1994 iluuu1994 marked this pull request as ready for review August 3, 2023 07:42
@iluuu1994 iluuu1994 requested a review from dstogov as a code owner August 3, 2023 07:42
We require valid code for compilation to succeed, but these paths should always
be guarded by OPx_TYPE checks and never execute. Add an assertion to verify.
@iluuu1994 iluuu1994 force-pushed the vm-ptr_ptr-assertions branch from 5eb1871 to 5493315 Compare August 3, 2023 09:10
Copy link
Member

@dstogov dstogov 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.

Could you please check the effect of this patch (e.g. size sapi/cli/php)
Does it reduce the code size?

@iluuu1994
Copy link
Member Author

Does it reduce the code size?

It didn't, it went from 28507008 to 28507168 with a ./configure --disable-debug --disable-all. That's an increase of 160 bytes.

@dstogov
Copy link
Member

dstogov commented Aug 3, 2023

Does it reduce the code size?

It didn't, it went from 28507008 to 28507168 with a ./configure --disable-debug --disable-all. That's an increase of 160 bytes.

This looks strange. I though this should remove some dead code paths. Try to take a look into disassemble of some of the affected handlers to understand what is going on.

@iluuu1994
Copy link
Member Author

iluuu1994 commented Aug 3, 2023

I see no difference in any of the assembly of any of the functions that now contain the zend_get_bad_ptr(). This sounds correct, as they should all already be guarded with a op_type check and thus eliminated already. Disassembling and comparing the entire binaries also shows no difference. I guess the difference comes from .rodata somewhere.

@dstogov
Copy link
Member

dstogov commented Aug 3, 2023

OK. merge this

@iluuu1994 iluuu1994 merged commit 73c5f36 into php:master Aug 3, 2023
jorgsowa pushed a commit to jorgsowa/php-src that referenced this pull request Aug 16, 2023
We require valid code for compilation to succeed, but these paths should always
be guarded by OPx_TYPE checks and never execute. Add an assertion to verify.
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.

2 participants