Skip to content

Fix build on Apple Clang 17+ #18629

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
May 24, 2025
Merged

Fix build on Apple Clang 17+ #18629

merged 2 commits into from
May 24, 2025

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented May 23, 2025

After upgrading my Xcode version to the one that includes Apple Clang 17.0.0, I got the following compilation errors:

In file included from php-src/ext/hash/hash_sha.c:20:
php-src/Zend/zend_cpuinfo.h:133:9: error: invalid cpu feature string for builtin [-Werror]
  133 |         return __builtin_cpu_supports("sse2");
      |                ^                      ~~~~~~
php-src/Zend/zend_cpuinfo.h:141:9: error: invalid cpu feature string for builtin [-Werror]
  141 |         return __builtin_cpu_supports("sse3");
      |                ^                      ~~~~~~
php-src/Zend/zend_cpuinfo.h:149:9: error: invalid cpu feature string for builtin [-Werror]
  149 |         return __builtin_cpu_supports("ssse3");
      |                ^                      ~~~~~~~
php-src/Zend/zend_cpuinfo.h:157:9: error: invalid cpu feature string for builtin [-Werror]
  157 |         return __builtin_cpu_supports("sse4.1");
      |                ^                      ~~~~~~~~
php-src/Zend/zend_cpuinfo.h:165:9: error: invalid cpu feature string for builtin [-Werror]
  165 |         return __builtin_cpu_supports("sse4.2");
      |                ^                      ~~~~~~~~
php-src/Zend/zend_cpuinfo.h:173:9: error: invalid cpu feature string for builtin [-Werror]
  173 |         return __builtin_cpu_supports("avx");
      |                ^                      ~~~~~
php-src/Zend/zend_cpuinfo.h:181:9: error: invalid cpu feature string for builtin [-Werror]
  181 |         return __builtin_cpu_supports("avx2");

In order to fix the errors, I added some preprocessor conditions that skip calling __builtin_cpu_supports with the unsupported features on an Apple ARM when using Apple Clang.

Alternatively, I could have used a configure script based approach (probably similarly to

dnl PHP_CHECK_AVX512_SUPPORTS
), but I didn't really know how to implement a similar check there... So at last I sticked with the original, maybe less elegant solution.

@@ -126,58 +126,86 @@ ZEND_API int zend_cpu_supports(zend_cpu_feature feature);
* functions */
ZEND_NO_SANITIZE_ADDRESS
static inline int zend_cpu_supports_sse2(void) {
#if (defined(__APPLE__) && defined(__aarch64__) && defined(__clang_major__) && __clang_major__ >= 17)
Copy link
Member

@nielsdos nielsdos May 24, 2025

Choose a reason for hiding this comment

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

Just checking for __aarch64__ should be enough and is simpler

Copy link
Member

Choose a reason for hiding this comment

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

Although ideally we would use an allowlust rather than a denylist but that requires knowing what cpu we build for for sure and not all compilers set it the same way

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Ok for me

@kocsismate kocsismate merged commit 772479e into php:PHP-8.3 May 24, 2025
9 checks passed
@kocsismate kocsismate deleted the clang-fix branch May 24, 2025 20:26
kocsismate added a commit that referenced this pull request May 24, 2025
* PHP-8.3:
  Fix build on Apple Clang 17+ (#18629)
kocsismate added a commit that referenced this pull request May 24, 2025
* PHP-8.4:
  Fix build on Apple Clang 17+ (#18629)
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