Skip to content

Stop copying zend_module_entry #8551

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

Jump to bottom
Merged
merged 1 commit into from
May 24, 2022
Merged

Stop copying zend_module_entry #8551

merged 1 commit into from
May 24, 2022

Conversation

morrisonlevi
Copy link
Contributor

@morrisonlevi morrisonlevi commented May 13, 2022 •
edited
Loading

I did some historical digging and could not figure out why a copy is
made. Additionally, the copy was not using the .size member, so it
was probably wrong, but it's been that way for quite some time. I
asked around and couldn't find anyone who knew more about it.

Tests passed locally so now I'm trying it out on CI. If all looks good
then I'll start testing some of the weird things like Apache reload and
restart.

@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented May 16, 2022 •
edited
Loading

I am getting crashes in Apache reloads on both ZTS and NTS* that are unrelated to this change and appear to be related to signal processing happening when SIGG globals are invalid. That's why my progress has stalled on this PR. However, I'm hopeful that Apache doesn't actually need this copy, and that we can land this at some point.

I have some -Werror fixes on this branch I'll probably pick out for master and merge separately.

* I think the NTS failure was just PEBKAC but the ZTS one is legitimate.

@morrisonlevi morrisonlevi force-pushed the stop-copying-zend-module-entry branch from c618cde to 2452002 Compare May 18, 2022 16:00
@morrisonlevi
Copy link
Contributor Author

I've rebased on the latest master which has the -Werror fixes.

@morrisonlevi morrisonlevi marked this pull request as ready for review May 20, 2022 15:53
@morrisonlevi
Copy link
Contributor Author

morrisonlevi commented May 20, 2022 •
edited
Loading

I've been unable to crash Apache on reload using Apache with Prefork MPM and PHP NTS. The ZTS issue prevents testing that Event MPM.

This is ready to go from my perspective if the tests pass from rebasing onto master.

@morrisonlevi
Stop copying zend_module_entry
84ef01e 
I did some historical digging and could not figure out why a copy is
made. Additionally, the copy was not using the `.size` member, so it
was probably wrong, but it's been that way for quite some time.
@morrisonlevi morrisonlevi force-pushed the stop-copying-zend-module-entry branch from 2452002 to 84ef01e Compare May 20, 2022 16:12
@ramsey ramsey added this to the PHP 8.2 milestone May 21, 2022
bwoebi
bwoebi approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

LGTM

@morrisonlevi morrisonlevi merged commit b63df3c into php:master May 24, 2022
@morrisonlevi morrisonlevi deleted the stop-copying-zend-module-entry branch May 24, 2022 15:13
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Sep 30, 2022
@arnaud-lb
Revert "Stop copying zend_module_entry (php#8551)"
81fb402 
This reverts commit b63df3c.
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request Sep 30, 2022
@arnaud-lb
Revert "Stop copying zend_module_entry (php#8551)"
09ab4ae 
This reverts commit b63df3c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwoebi bwoebi bwoebi approved these changes

Assignees
No one assigned
Labels
Category: Engine Waiting on Review
Projects
None yet
Milestone
PHP 8.2
Development

Successfully merging this pull request may close these issues.
3 participants
@morrisonlevi @bwoebi @ramsey