Skip to content

Make stripslashes() only dependent on SSE2 configuration. #10408

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
Jan 23, 2023

Conversation

nielsdos
Copy link
Member

Alex Dowad noticed[1] that the SIMD stripslashes implementation actually only depended on SSE2, and not on SSE4.2 instructions. Remove the checking for SSE4.2 and only check for SSE2. This also greatly simplifies the supporting code.

I thought I'd make a PR since @alexdowad mentioned he still has a lot of other stuff to do [2].

[1] #10313 (comment)
[2] #10313 (comment)

@alexdowad
Copy link
Contributor

Thanks so much for working on this, @nielsdos.

@alexdowad
Copy link
Contributor

The code looks good to me. It is indeed a big simplification.

I am interested in learning more about what __attribute__((ifunc("resolve_stripslashes"))) was for. I can see that we had a global function pointer to the implementation being used, this pointer was set in the module init function, then calls to the function go through that pointer. Why also have the attribute as well?

Alex Dowad noticed[1] that the SIMD stripslashes implementation actually
only depended on SSE2, and not on SSE4.2 instructions. Remove the
checking for SSE4.2 and only check for SSE2. This also greatly
simplifies the supporting code.

[1] php#10313 (comment)
@nielsdos
Copy link
Member Author

It was necessary to resolve which function to use at link time: either the SSE4.2 one or the normal one. You can read more about it here: https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html (CTRL+F for ifunc ("resolver")).

@alexdowad
Copy link
Contributor

It was necessary to resolve which function to use at link time: either the SSE4.2 one or the normal one. You can read more about it here: https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html (CTRL+F for ifunc ("resolver")).

But then why also have the global function pointer which is set in the module init function? Is it so that the best host-specific implementation will be chosen even if PHP is linked against a different libc, or against an old version of glibc which doesn't support ifunc?

@nielsdos
Copy link
Member Author

But then why also have the global function pointer which is set in the module init function? Is it so that the best host-specific implementation will be chosen even if PHP is linked against a different libc, or against an old version of glibc which doesn't support ifunc?

Yes. In case the dynamic linker / libc does not support the link-time resolution method, then it can fallback to the global function pointer.

@Girgias Girgias merged commit 2b55dee into php:master Jan 23, 2023
@Girgias
Copy link
Member

Girgias commented Jan 23, 2023

Thank you!

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