Don't try to use C11 atomics, which are not const #11931
Conversation
C11 did not have the const qualifier on atomic_load; C17 does. One can either use C11 atomics by casting away the constness, which was proposed and rejected in phpGH-8764, or one can avoid using C11 atomics altogether, instead using C17 atomics if available, which is what this change does. Fixes phpGH-8881
|
So C17 really is a bug fix release of C11... One "solution" is to define a macro the casts away the const qualifier for C11 atomics and is a no-op for C17 compliant compilers. This might be more acceptable as otherwise I don't know if other people are going to be fine to bump from C99 to C17 in one go due to whatever compiler requirements we have. (I think it is every supported distro must ship with a Cxy capable compiler by default) |
Note that my change does not change the PHP compiler requirements. PHP still compiles fine with a C99 compiler. The only change is that if you have a C17-incapable compiler in C11 mode the build will not fail because it won't try to use those atomics. And if you have a C17-capable compiler in C11 mode it will not use those atomics whereas before it would. I'm not sure how important the use of atomics is. |
|
How shall we proceed with this issue? It would be nice if users with a particular vintage of compilers did not experience build failures, which is what happens without this patch. |
| #if __has_feature(c_atomic) | ||
| #define HAVE_C11_ATOMICS 1 | ||
| #if __has_feature(c_atomic) && defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201710L | ||
| #define HAVE_C17_ATOMICS 1 |
There was a problem hiding this comment.
This wouldn't be suitable for PHP 8.2. Can you share your compiler, compiler version, and target platform? We'll need to work out some detection for whether we need to cast the const away for PHP 8.2 (but for 8.3 we can probably apply this patch).
There was a problem hiding this comment.
I've updated the linked issue with the compiler and OS information.
What makes you say this wouldn't be suitable for PHP 8.2? I have already applied this patch to MacPorts PHP 8.2 and 8.3 and have not received any reports of problems, though of course that doesn't guarantee there aren't any problems.
There was a problem hiding this comment.
We cannot just take away HAVE_C11_ATOMICS existing in a patch release.
There was a problem hiding this comment.
If your objection is about changing the name of the macro from HAVE_C11_ATOMICS to HAVE_C17_ATOMICS, then a solution could be not to change the name.
There was a problem hiding this comment.
I renamed it in order to have the macro name reflect the change in its definition. I did not consider that it might be part of public API that others would use. If it is public API, then another option might be to leave HAVE_C11_ATOMICS defined as it was and just not use it in PHP itself, addition to creating HAVE_C17_ATOMICS and using it as in this PR.
Of course, I don't care how it's fixed, and I'm not very familiar with the code of the PHP project so I don't know what fix is best. All I care about is that it is fixed somehow.
C11 did not have the
constqualifier onatomic_load; C17 does. One can either use C11 atomics by casting away theconstness, which was proposed and rejected in GH-8764, or one can avoid using C11 atomics altogether, instead using C17 atomics if available, which is what this change does.Fixes GH-8881
This issue only affects the PHP 8.2 and later branches.
This issue only affects compilers in C11 mode that do not support C17. Compilers that do support C17 behave as though the
constqualifier was present even when in C11 mode. It might only affect clang.I'm not sure if I've written a satisfactory commit message. If not, feel free to reword it or let me know what needs to be changed.