Re: [PHP-DEV] Require C11 in PHP 8.4

On Thu, 1 Aug 2024 at 23:57, Ilija Tovilo <tovilo.ilija@gmail.com> wrote:

[…]
I started fixing these in a PR [1] which required more changes than
expected. After a short discourse, we were wondering whether it might
be better to switch to a newer C standard instead. Our coding
standards [2] currently specify that compiling php-src requires C99.
The Unix installation page on php.net [3] claims it is ANSI C, which
is certainly outdated. There have been suggestions to require C11 for
a while, which should be well supported by all compilers shipped with
maintained distributions.

It feels wrong to raise such an important requirement that might affect a lot of people, including maintainers of extensions, for just one specific problem, and 99% of the codebase would still be C99 compliant.

I quickly put together an alternative PR (#15202) with a slightly different approach, just as a proof of concept. The idea is to move all the global typedefs in a new include header “zend_types_defs.h” (but also zend_portability.h can be reused for this purpose, as all the relevant files already include it).

While putting together that PR, I had the feeling that this typedef redefinition problem is in reality hiding some smelly design of header files. So maybe, rather than requiring a compiler more tolerant to poor code, we should rather focus on getting the design right.

Hi,

On Fri, Aug 2, 2024 at 1:36 PM Giovanni Giacobbi <giovanni@giacobbi.net> wrote:

It feels wrong to raise such an important requirement that might affect a lot of people, including maintainers of extensions

Why do you think that this might affect a lot of people? We are talking about minimum GCC version 4.7 and really old Clang version (that probably no one uses) as well. And minimum MSVC is already 2019. Do you know about some specific platforms that would be affected by this?

Cheers

Jakub

On Friday, 2 August 2024 at 14:33, Giovanni Giacobbi <giovanni@giacobbi.net> wrote:

On Thu, 1 Aug 2024 at 23:57, Ilija Tovilo <tovilo.ilija@gmail.com> wrote:

[...]
I started fixing these in a PR [1] which required more changes than
expected. After a short discourse, we were wondering whether it might
be better to switch to a newer C standard instead. Our coding
standards [2] currently specify that compiling php-src requires C99.
The Unix installation page on php.net [3] claims it is ANSI C, which
is certainly outdated. There have been suggestions to require C11 for
a while, which should be well supported by all compilers shipped with
maintained distributions.

It feels wrong to raise such an important requirement that might affect a lot of people, including maintainers of extensions, for just one specific problem, and 99% of the codebase would still be C99 compliant.

I quickly put together an alternative PR (#15202) with a slightly different approach, just as a proof of concept. The idea is to move all the global typedefs in a new include header "zend_types_defs.h" (but also zend_portability.h can be reused for this purpose, as all the relevant files already include it).

While putting together that PR, I had the feeling that this typedef redefinition problem is in reality hiding some smelly design of header files. So maybe, rather than requiring a compiler more tolerant to poor code, we should rather focus on getting the design right.

I would like to see actual factual data rather than "vibes" as to how this would cause issues with third-party extensions.
php-src is not C99 pedeantic compliant and we do use GCC extenstions.

I do agree that maybe we should go back to fixing headers, but considering the drama this caused 18 months ago I do not know who here has the motivation to do this.

Ideally I feel we should target C17 as it is a "bug fix" release of C11, but this might be impossible due to old compilers still being the default on LTS Distributions.
The main benefit of C11/17 in the long run are atomics, that we kinda use already anyway.

Moreover, as someone that has written an extension that has some usage, when I go back to it, I'd rather like to be able to use features from C23 than be stuck on C99, but that is just me.

Best regards,
Gina P. Banyard

On Sun, Aug 4, 2024 at 10:05 AM Gina P. Banyard <internals@gpb.moe> wrote:

On Friday, 2 August 2024 at 14:33, Giovanni Giacobbi <giovanni@giacobbi.net> wrote:

On Thu, 1 Aug 2024 at 23:57, Ilija Tovilo <tovilo.ilija@gmail.com> wrote:

[...]
I started fixing these in a PR [1] which required more changes than
expected. After a short discourse, we were wondering whether it might
be better to switch to a newer C standard instead. Our coding
standards [2] currently specify that compiling php-src requires C99.
The Unix installation page on php.net [3] claims it is ANSI C, which
is certainly outdated. There have been suggestions to require C11 for
a while, which should be well supported by all compilers shipped with
maintained distributions.

It feels wrong to raise such an important requirement that might affect a lot of people, including maintainers of extensions, for just one specific problem, and 99% of the codebase would still be C99 compliant.

I quickly put together an alternative PR (#15202) with a slightly different approach, just as a proof of concept. The idea is to move all the global typedefs in a new include header "zend_types_defs.h" (but also zend_portability.h can be reused for this purpose, as all the relevant files already include it).

While putting together that PR, I had the feeling that this typedef redefinition problem is in reality hiding some smelly design of header files. So maybe, rather than requiring a compiler more tolerant to poor code, we should rather focus on getting the design right.

I would like to see actual factual data rather than "vibes" as to how this would cause issues with third-party extensions.
php-src is not C99 pedeantic compliant and we do use GCC extenstions.

I do agree that maybe we should go back to fixing headers, but considering the drama this caused 18 months ago I do not know who here has the motivation to do this.

Ideally I feel we should target C17 as it is a "bug fix" release of C11, but this might be impossible due to old compilers still being the default on LTS Distributions.
The main benefit of C11/17 in the long run are atomics, that we kinda use already anyway.

As a person who wrote some of those atomics, I would definitely prefer
to move to standard C11/C17 atomics at some point in the future and
remove all other fallbacks. The blocker right now is Microsoft Visual
Studio. Although they added C11/C17 support a few versions back, they
did not add atomics at that time. It's available now, but it is still
experimental as far as I can tell.

Maybe for PHP 9.0 we can address header issues and also by then VS
will have a general release we can move to for atomics.

One thing I feel strongly about is that we should not add flags like
`-std=c11`. If we check, we ought to just ensure that whatever flags
the user provided, we have certain C11/C17 features. This allows for
extensions to use a newer version, or a relaxed version like
`-std=gnu11`.

On 05.08.2024 at 18:42, Levi Morrison wrote:

As a person who wrote some of those atomics, I would definitely prefer
to move to standard C11/C17 atomics at some point in the future and
remove all other fallbacks. The blocker right now is Microsoft Visual
Studio. Although they added C11/C17 support a few versions back, they
did not add atomics at that time. It's available now, but it is still
experimental as far as I can tell.

There is a MS blog post from December, 2022[1] which claims that it is
experimental, and that you would need to specify the
/experimental:c11atomics flag in addition to /std:c11 to enable atomics
support. I though, hey, why not try that out, added only /std:c11 and
got a failing minimal build, because apparently max_align_t is not
defined in stddef.h even with the latest available SDK (10.0.26100.0)
offered by Visual Studio 2022 for my Windows 10 machine. It might be
available with SDK 10.0.22621.2428, which according to the
documentation[2] has been "Relesed in October 2024"[sic]. I wonder
whether I would trust more detailed information there.

(FWIW, the official Windows builds use 10.0.22621.0, and we probably
should enforce some common version; I think for now we use just what can
be found in the build environment.)

Anyway, I worked around the missing max_align_t definition, and the
build succeeded, even though I've added

#ifndef __STDC_NO_ATOMICS__
# error cmb
#endif

in zend_API.c. Thus, I conclude that atomics support in latest Visual
Studio 17.10.5 is no longer deemed experimental, and locking atomics are
implemented now. I think it is worth investigating whether that support
actually works for our purposes, but probably not use that for PHP 8.4.

[1]
<https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/&gt;
[2] <https://developer.microsoft.com/en-us/windows/downloads/sdk-archive/&gt;

Christoph