-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Check for compiler typedef redeclaration support at configure-time #15397
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Peter Kokot <peterkokot@gmail.com>
Co-authored-by: Peter Kokot <peterkokot@gmail.com>
Co-authored-by: Peter Kokot <peterkokot@gmail.com>
Thanks Peter. At this point I should just change the author to you. 😄 |
Ah, no, that's no problem... Comments are like a tutorial here. |
See PR #15403 for how this could be ensured for Visual Studio builds (I'll follow up with a patch for clang on Windows in due course). |
What about also adding the For example, if clang is used: This will produce error:
This will build without warnings or errors:
|
@petk I'd be ok with that. But that does mean potentially catching less errors at configure-time when eventually using more C11 feature on master. Also, it seems |
Yes, adding this flag needs to be done conditionally with the AX_CHECK_COMPILE_FLAG. It resolves this specific case: PHP being built in C99 standard with additional typedef redefinitions from C11. It apllies only for Clang, as GCC versions newer than ~4.2 don't emit these warnings. And these warnings start to happen only when using the -std compile option on Clang. So it's like customizing the C99 standard. Regarding the initial issue, it's the compiler gcc 4.2 that somehow is configured with c89 by default and it doesn't support the typedef redefinitions. So, there's no other way but to either pass some form of -std compile option to override the compiler defaults, or to do a fatal error to upgrade the compiler (and the -std flag probably also wouldn't resolve this specific compiler configuration, I think.) |
And also for people being worried, that this approach is a bad practice: it is not. Fixing the code to not have typedef redefinitions would be considered a bad practice. Because typedef redefinitions are completely valid programming in C11. Fixing this in the code at this point would be like going backwards, while C11 is few steps away from being accepted as PHP coding standard. It's that simple. Also many other open source projects have taken this path: allowing typedef redefinitions, while being C99 compliant. |
Wouldn't that already be an issue? We're using ample C99 features. And gcc doesn't support |
This potentially fixes some Clang builds on some specific configurations: diff --git a/Zend/Zend.m4 b/Zend/Zend.m4
index b7b44fb140..f137a6d826 100644
--- a/Zend/Zend.m4
+++ b/Zend/Zend.m4
@@ -206,6 +206,10 @@ AX_CHECK_COMPILE_FLAG([-Wstrict-prototypes],
AX_CHECK_COMPILE_FLAG([-fno-common],
[CFLAGS="-fno-common $CFLAGS"],,
[-Werror])
+dnl Suppress the Clang typedef redefinition warnings if it has enabled C99 mode.
+AX_CHECK_COMPILE_FLAG([-Wno-typedef-redefinition],
+ [CFLAGS="$CFLAGS -Wno-typedef-redefinition"],,
+ [-Werror])
ZEND_CHECK_ALIGNMENT
ZEND_CHECK_SIGNALS Basically, it's the same issue as with that GCC 4.2. If Clang has C99 mode enabled (probably it will be the gnu99), then the typedef redefinition warnings will show up in the build logs. And even worse, if the build stack is set to errors on certain warnings. |
This allows it to continue to run in C99 mode.
@petk Ok, I've added the suggested change. |
Basically this is ok, except that Autoconf also does some checks. And newer the Autoconf version, newer the C standards conforms checks are. The Autoconv 2.72 does C11, C99 and C89 checks and the unreleased Autoconf does also C23 check if compiler command needs to be adjusted somehow. This is done automatically by the AC_PROG_CC. :) I'll recheck this a bit further. |
@petk Thank you! Feel free to push to the PR directly. |
Hm, I've just found another bug. Checking the |
Ok, I've checked this a bit further. So, correct me if I'm wrong here. The typedef redefinitions were added into GCC 4.6 and later: So if we turn this a bit around and return to the previous discussions, this will also make the GCC version 4.6 as a minimum requirement for PHP. :D (cc @cmb69) At least as an undocumented minimum GCC requirement. And about these diff --git a/Zend/Zend.m4 b/Zend/Zend.m4
index 7ffdfcbf9b..3bd22f355c 100644
--- a/Zend/Zend.m4
+++ b/Zend/Zend.m4
@@ -231,7 +231,7 @@ AX_CHECK_COMPILE_FLAG([-fno-common],
[CFLAGS="-fno-common $CFLAGS"],,
[-Werror])
dnl Suppress the Clang typedef redefinition warnings if it has enabled C99 mode.
-AX_CHECK_COMPILE_FLAG([-Wno-typedef-redefinition],
+AX_CHECK_COMPILE_FLAG([-Wtypedef-redefinition],
[CFLAGS="$CFLAGS -Wno-typedef-redefinition"],,
[-Werror]) However, probably from this build system point of view this check doesn't make things so clear why it's here. And we haven't encountered such issue yet. Also, from what I was thinking here this -Wtypedef-redefinition check should be then done before the typedef check and not causing the Clang cause fatal error... I'm still rechecking further what would be appropriate way here. |
In my opinion, this is not a real problem. After all, gcc 4.6.0 has been released on March 25, 2011, and users still stuck with older compilers should not expect to be able to build recent software releases with it. There was a somewhat related discussion about PHP 8.3 having dropped support for Windows 7, and that has been shut down for the same reasons. |
About the Autoconf checks: There is this For example, With Autoconf 2.72 the C11 standard will be used if compiler supports it. Otherwise, it will try with C99 and lastly with C89. The upcoming Autoconf version will most likely even do something in the C23 direction from what I see in the code. At least it does some C23 checks.
Hm, by bumping standard to C11 also Autoconf minimum version should (ideally) be bumped so someone won't have issues with generating configure script with an outdated C99-only Autoconf version. The Autoconf 2.70 has C11 "support". Still checking this further... :) |
Apple gcc 4.2.1 and presumably FSF gcc 4.2.1 on which it is based is in gnu89 mode by default and can be put into c89, c99, or gnu99 mode using the |
Just for the info, there is another typedef redefinition warning appearing in ext/opcache/jit/ir/ir_private.h:841:3 redefinition of typedef 'ir_hashtab' ... (meaning the flag -Wno-typedef-redefinition should be added also there somewhere when building the IR hash fold generator executable) |
Closes GH-15070
As per usual wrt. autoconf, I have no clue what I'm doing. Please check if this makes sense. :)