I agree that this is a bug, but I'm not sure whether phpize is prepared for this. In particular we'll end up including both the extension config.h (typically first, I think) and the main php_config.h and will get symbol clashes at that point.

I think for stable releases we should just add an explicit exclude for old clang versions (i.e. set ZEND_USE_ASM_ARITHMETIC=0 if HAVE_ASM_GOTO=1 but compiler too old), because I think that's the main practical problem here.

I'm thinking something like this:

diff --git a/Zend/zend_operators.h b/Zend/zend_operators.h
index dad23bc4d8..a38b651ff8 100644
--- a/Zend/zend_operators.h
+++ b/Zend/zend_operators.h
@@ -500,7 +500,8 @@ ZEND_API void zend_update_current_locale(void);
 #define ZVAL_OFFSETOF_TYPE     \
        (offsetof(zval, u1.type_info) - offsetof(zval, value))
 
-#if defined(HAVE_ASM_GOTO) && !__has_feature(memory_sanitizer)
+#if defined(HAVE_ASM_GOTO) && !__has_feature(memory_sanitizer) \
+       && (!defined(__clang_major__) || __clang_major >= 9)
 # define ZEND_USE_ASM_ARITHMETIC 1
 #else
 # define ZEND_USE_ASM_ARITHMETIC 0

You have a typo where __clang_major should be __clang_major__. And to check whether you're dealing with clang, usually you would check whether __clang__ is defined. (If it is, then surely __clang_major__ is defined too.)

But I would caution you that open source clang and Apple clang are two different compilers with different version numbering schemes. A feature that becomes available in open source clang 9 (released September 2019) is not necessarily available in Apple clang 9 (released in Xcode 9 in September 2017) and vice-versa. If you want to check the clang version, then you should first check whether __apple_build_version__ is defined. If it is, it's Apple clang and your comparisons of __clang_major__ should be about Apple clang major versions, otherwise they should be about open source clang major versions. If you need finer granularity for checking Apple clang versions, you could check the value of __apple_build_version__. For example, Apple clang 9 has an __apple_build_version__ greater than or equal to 9000000 and less than 10000000.

We have a list of Apple clang version numbers in each Xcode version if that's helpful: https://trac.macports.org/wiki/XcodeVersionInfo

I believe support for asm goto was introduced in open source clang 9 (https://github.com/llvm/llvm-project/commit/784929d0454c4df6a98ef6fbbd1d30a6f71f9c16) but I don't know in which version of Apple clang it was introduced. But support appears to be absent in Apple clang 11.0.3 (__apple_build_version__ 11030032 from Xcode 11.7) and present in Apple clang 12.0.5 (__apple_build_version__ 12050022 from Xcode 12.5). My guess would be that support was added in Apple clang 12.0.0 (Xcode 12.0) in which case what you want is:


--- a/Zend/zend_operators.h
+++ b/Zend/zend_operators.h
@@ -500,7 +500,8 @@ ZEND_API void zend_update_current_locale(void);
 #define ZVAL_OFFSETOF_TYPE     \
        (offsetof(zval, u1.type_info) - offsetof(zval, value))
 
-#if defined(HAVE_ASM_GOTO) && !__has_feature(memory_sanitizer)
+#if defined(HAVE_ASM_GOTO) && !__has_feature(memory_sanitizer) \
+       && (!defined(__clang__) || (defined(__apple_build_version__) && __clang_major__ >= 12) || __clang_major__ >= 9)
 # define ZEND_USE_ASM_ARITHMETIC 1
 #else
 # define ZEND_USE_ASM_ARITHMETIC 0


but I haven't tested this. And it seems a little troubling to me to bother having a HAVE_ASM_GOTO define if we're going to second-guess its value.

Given the version mess, it's probably better to just go with "&& !defined(__clang__)". Clang codegen using __builtin_saddl_overflow() isn't optimal, but pretty reasonable: https://c.godbolt.org/z/c3M86s7Tv

> And it seems a little troubling to me to bother having a HAVE_ASM_GOTO define if we're going to second-guess its value.

Right. However, this seems like the only safe choice for stable releases.

For master, we could mess with the build system, but I'm not clear on how that would look like. As mentioned, we'd have to deal with two different config.h files with conflicting information, and I don't know what the proper way to resolve that would be.

A possible option would be to move the assembly implementations into a separate header, so that not every single CU needs to include them. It would still have to be an exported header because opcache JIT uses it, but apart from that these implementations shouldn't really be relevant to extensions.

I am grateful to you and expect more number of posts like these. Thank you very much. (https://github.com)(https://www.iadpworkforcenow.com)