Skip to content
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

Fix detection of unknown gcc function attributes #8483

Closed
wants to merge 1 commit into from

Conversation

athos-ribeiro
Copy link
Contributor

@athos-ribeiro athos-ribeiro commented May 2, 2022

As described in autoconf-archive upstream [1], from where
build/ax_gcc_func_attribute.m4 is forked, the old unknown func attr
detection method would throw a false negative anytime an unrelated
warning was raised.

This results in ax_cv_have_func_attribute_target being set to no
whenever certain compiler Warning flags are switched on. Namely, having
-Wall on, which is a default CFLAG for some linux distributions, will
result in

warning: ‘bar’ declared ‘static’ but never defined [-Wunused-function]

when evaluating support for the target function attribute.

With that configuration value set to no, the compiled php binaries
will not support x86_64 v3 instructions such as avx2 and sse2, which
should speed up specific tasks ran by PHP.

This issue was originally reported in Ubuntu [2].

[1] http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=df0894ad1a8195df67a52108b931e07d708cec9a
[2] https://bugs.launchpad.net/ubuntu/+source/php8.1/+bug/1882279

As described in autoconf-archive upstream [1], from where
`build/ax_gcc_func_attribute.m4` is forked, the old unknown func attr
detection method would throw a false negative anytime an unrelated
warning was raised.

This results in `ax_cv_have_func_attribute_target` being set to `no`
whenever certain compiler Warning flags are switched on. Namely, having
`-Wall` on, which is a default CFLAG for some linux distributions, will
result in

```
warning: ‘bar’ declared ‘static’ but never defined [-Wunused-function]
```

when evaluating support for the `target` function attribute.

With that configuration value set to `no`, the compiled php binaries
will not support x86_64 v3 instructions such as avx2 and sse2, which
should speed up specific tasks ran by PHP.

This issue was originally reported in Ubuntu [2].

[1] http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=commitdiff;h=df0894ad1a8195df67a52108b931e07d708cec9a
[2] https://bugs.launchpad.net/ubuntu/+source/php8.1/+bug/1882279
@cmb69
Copy link
Contributor

cmb69 commented May 3, 2022

Thank you for the PR, and the thorough explanation!

With that configuration value set to no, the compiled php binaries
will not support x86_64 v3 instructions such as avx2 and sse2, which
should speed up specific tasks ran by PHP.

Ugh, that is bad.

I think this PR should target the PHP-8.0 branch, and we may consider to update all autoconf files copyied from upstream to the latest revisions for the master branch.

@athos-ribeiro athos-ribeiro changed the base branch from master to PHP-8.0 May 3, 2022
@athos-ribeiro
Copy link
Contributor Author

athos-ribeiro commented May 3, 2022

Thank you for the PR, and the thorough explanation!

With that configuration value set to no, the compiled php binaries
will not support x86_64 v3 instructions such as avx2 and sse2, which
should speed up specific tasks ran by PHP.

Ugh, that is bad.

I think this PR should target the PHP-8.0 branch, and we may consider to update all autoconf files copyied from upstream to the latest revisions for the master branch.

I based it in the php-8.0 branch but forgot to target it in this PR. Fixed.

Should I go ahead and update the other macros in this PR or would it be OK to file another PR for that matter?

@cmb69
Copy link
Contributor

cmb69 commented May 3, 2022

Should I go ahead and update the other macros in this PR or would it be OK to file another PR for that matter?

Please submit a separate PR for this, and target the "master" branch. This very PR is about fixing a bug; the full update might cause issues for the stable branches.

cmb69
cmb69 approved these changes May 3, 2022
Copy link
Contributor

@cmb69 cmb69 left a comment

This looks good to me. Some *nix devs may like to review.

Copy link
Contributor

@devnexen devnexen left a comment

LGTM

@devnexen devnexen closed this in 813d942 Jun 2, 2022
@devnexen
Copy link
Contributor

devnexen commented Jun 2, 2022

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants