Skip to content

fix tailcall VM for gcc - #23603

Open
henderkes wants to merge 4 commits into
php:masterfrom
henderkes:feat/gcc-tailcall
Open

fix tailcall VM for gcc#23603
henderkes wants to merge 4 commits into
php:masterfrom
henderkes:feat/gcc-tailcall

Conversation

@henderkes

Copy link
Copy Markdown
Contributor

I recall seeing an issue about this, but it seems it just disabled tailcall vm selection instead of patching the only conflicting function

@henderkes

Copy link
Copy Markdown
Contributor Author

cc @arnaud-lb for review

@TimWolla
TimWolla requested a review from arnaud-lb September 6, 2026 20:51
@arnaud-lb

Copy link
Copy Markdown
Member

The related issue is #23054.

This builds successfully with your changes, with GCC 16, if we ignore a bunch of maybe-musttail-local-addr warnings. My memory is fuzzy, but this warning might have been an error in the past? We should verify that we are not breaking the build for older GCC versions. We could add a pragma to ignore that error as well.

Otherwise the change looks good. We should probably apply it to all compilers as this shouldn't hurt, and the code would be easier to follow.

Please target master only.

@henderkes
henderkes changed the base branch from PHP-8.5 to master September 7, 2026 11:54
@henderkes

Copy link
Copy Markdown
Contributor Author

GCC only added preserve_none support in gcc 16 so it can't break for earlier gcc versions, they couldn't arrive in the branch.

I'm not sure if we should be silencing the warnings, assuming they are legitimate it may be worth finding workarounds later on. But that's for another time.

@henderkes

Copy link
Copy Markdown
Contributor Author

Otherwise the change looks good. We should probably apply it to all compilers as this shouldn't hurt, and the code would be easier to follow.

Technically it's an extra function call in the one-time runtime compilation on clang, but it really shouldn't matter for performance.

@arnaud-lb arnaud-lb left a comment

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.

This looks good to me! Do you want to add NEWS entry? I will merge after that

@henderkes

Copy link
Copy Markdown
Contributor Author

Done!

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