php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #81124 Determination of asm goto capability should be made by phpize configure script
Submitted: 2021-06-10 14:35 UTC Modified: 2021-06-14 19:45 UTC
Votes:2
Avg. Score:4.0 ± 1.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:1 (100.0%)
From: php-bugs-2021 at ryandesign dot com Assigned:
Status: Open Package: *Compile Issues
PHP Version: 8.0.7 OS: macOS
Private report: No CVE-ID: None
 [2021-06-10 14:35 UTC] php-bugs-2021 at ryandesign dot com
Description:
------------
php 7.3.? and later use asm goto in zend_operators.h if HAVE_ASM_GOTO is set in php_config.h. A problem arises if the compiler that was used to compile php supports asm goto but the compiler used to compile a separate php module (with phpize) does not support asm goto. The error message in that case is e.g.:

/opt/local/include/php73/php/Zend/zend_operators.h:523:10: error: expected '(' after 'asm'
        __asm__ goto(
                ^

See https://trac.macports.org/ticket/62022 for the way that this affects php 7.3.x in MacPorts today, but note that as far as I know the potential for similar issues continues to exist in all later php versions too.

A previous bug report about this issue #75951 was closed as not a bug because of the difference in compilers. While perhaps you would prefer that all modules be compiled with the same compiler that was used to compile php, that cannot always be assured. For example, maybe a user compiles php, then changes their compiler (on macOS, maybe the user changes their Xcode version -- maybe it is updated for them automatically by Apple Software Update), then compiles a php module. Or maybe the choice of compiler is dictated by deficiencies in or requirements of a particular module (e.g. some require a C++11-capable compiler and some don't).

php modules compiled with phpize get their own configure scripts, so presumably it was already recognized that certain determinations must be made anew for each module. Would it be possible to move or copy the determination of whether the compiler supports asm goto into this phpize-generated configure script?


Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-06-10 15:40 UTC] nikic@php.net
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.
 [2021-06-14 09:29 UTC] nikic@php.net
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
 [2021-06-14 17:14 UTC] php-bugs-2021 at ryandesign dot com
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.
 [2021-06-14 19:45 UTC] nikic@php.net
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.
 [2024-06-18 11:37 UTC] gregory109chavez at gmail dot com
I am grateful to you and expect more number of posts like these. Thank you very much. (https://github.com)(https://www.iadpworkforcenow.com)
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Oct 04 06:01:26 2024 UTC