Skip to content

Stop copying internal functions into each thread #10517

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 2 commits into from
Feb 13, 2023

Conversation

dstogov
Copy link
Member

@dstogov dstogov commented Feb 6, 2023

It seems we don't copy internal methods for a long time, so this shouldn't be a problem. We had to copy functions in PHP-5 times, but it seems we just forgot to remove this.

It's possible that some third-part extensions (e.g. profilers, tracers, debuggers) modify internal functions. After this change that may cause race conditions in ZTS build (but we already have the same behavior for internal methods). Observer API should provide the necesssary functionality to avoid shared structures modification.

It seems we don't copy internal methods for a long time, so this
shouldn't be a problem. We had to copy functions in PHP-5 times, but it
seems we just forgot to remove this.

It's possible that some third-part extensions (e.g. profilers, tracers,
debuggers) modify internal functions. After this change that may cause
race conditions in ZTS build (but we already jave the same behavior for
internal methods). Observer API should provide necesssary functionality
to avoid shared structures modification.
@bwoebi
Copy link
Member

bwoebi commented Feb 6, 2023

I support this. In fact I thought this was already the case :-D

Also, it does not even make sense to have the behavior separate from classes, where we actually already reference the global class pointer, and thereby indirectly provide access to the global zend_internal_function pointer on the class methods.

@dstogov
Copy link
Member Author

dstogov commented Feb 6, 2023

In fact I thought this was already the case :-D

I also was surprised :)

@bwoebi
Copy link
Member

bwoebi commented Feb 6, 2023

@dstogov Given that it fixes an actual crash (#10473) and given that third party extensions cannot modify internal methods arbitrarily at runtime, I assume the impact of this change is low enough and the benefit high enough to be merged into PHP 8.1 as well. It would be extremely surprising to me if any extension had particular behaviour for internal functions, but not internal methods.

@dstogov
Copy link
Member Author

dstogov commented Feb 6, 2023

I assume the impact of this change is low enough and the benefit high enough to be merged into PHP 8.1 as well.

I afraid, we can't merge this into old versions. This definitely may change behavior of some third-party extensions (even if they are already broken).

@dstogov dstogov requested review from iluuu1994 and derickr February 6, 2023 14:59
@dstogov
Copy link
Member Author

dstogov commented Feb 6, 2023

May be some one would propose a more clean way to do the same?

@dstogov dstogov requested a review from cmb69 February 6, 2023 15:01
@dstogov dstogov merged commit 3b75f07 into php:master Feb 13, 2023
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.

None yet

2 participants