Avoid C99 typedef redeclaration error #15079

iluuu1994 · 2024-07-23T16:10:07Z

petk · 2024-07-23T16:18:28Z

I wonder if there might be some more issues related to this typedef redefinition warnings (or errors on some compiler configurations). #13347 Another one, I've also seen in the ext/dom and I think one in ext/opcache somewhere but if the build on that macOS with GCC 4.2 works out then that's fine I guess. I'll also retest it with my build system a bit.

iluuu1994 · 2024-07-23T16:20:29Z

I do think we should seriously consider bumping the compiler requirement to C11 soon. We should do so before investing too much time here.

cmb69 · 2024-07-23T16:27:41Z

@barracuda156, could you please check if this compiles with the old GCC?

iluuu1994 · 2024-07-23T16:28:59Z

That would be great. I wasn't actually able to test this, -std=c99 alone is not enough to trigger an error.

cmb69

This looks good to me; a different solution might be to include zend_types.h instead of zend_portability.h.

cmb69 · 2024-07-23T18:01:57Z

I do think we should seriously consider bumping the compiler requirement to C11 soon.

Well, I just looked and couldn't even find any user targeted information on the C99 requirement (neither in the docs which say you need an "An ANSI C compiler", nor in README.md). Maybe this should be fixed first, before bumping the compiler requirements to C11 (although I don't have strong feelings about this).

iluuu1994 · 2024-07-23T18:03:56Z

@cmb69 It's here, quite hidden.

php-src/CODING_STANDARDS.md

Line 12 in e63f822

1. PHP is implemented in C99.

Edit: Oh, you edited that, so you're probably aware. Yes, that's not particularly user-targeted, and yes https://www.php.net/manual/en/install.unix.php is very much outdated.

morrisonlevi · 2024-07-23T19:33:28Z

I think this small PR is okay. But honestly, the user's OS is far-outdated, from 2009. We should not spend significant effort maintaining something so old.

TimWolla · 2024-07-23T20:53:05Z

Zend/zend_property_hooks.h

-typedef struct _zend_object_iterator zend_object_iterator;
-typedef struct _zval_struct zval;
+/* Intentionally avoid typedef because of C99 redeclaration errors. */
+struct _zend_array;


Seeing this, I already wondered before: Why is the struct tag even prefixed with an underscore instead of matching the typedef'd name exactly?

I think this is just an convention to avoid having identical names. And maybe also as a hint that clients should use the typedef'd name.

cmb69 · 2024-07-23T21:11:55Z

@morrisonlevi, for me, this is not about supporting old compilers and build tools, but to stick to the given standard (C99). There may be even new compilers who complain about this, at least when advised (e.g. https://clang.llvm.org/docs/DiagnosticsReference.html#wtypedef-redefinition).

iluuu1994 · 2024-07-23T21:59:04Z

There may be even new compilers who complain about this, at least when advised

@cmb69 Even though this is enabled by default, Clang only warns about this with -std=c99.

Maybe I misunderstood your statement, but if we correctly supplied the -std=c11 flag when bumping to C11, there should be no C99 warnings emitted by compilers supporting C11.

barracuda156 · 2024-07-24T10:39:36Z

@barracuda156, could you please check if this compiles with the old GCC?

@cmb69 Sure, will do it soon and update you.

iluuu1994 · 2024-07-24T10:40:58Z

@barracuda156 Please wait for a few more hours. I noticed that there are many more new errors like this. I'll update this PR.

C99 uses __asm__

barracuda156 · 2024-07-24T10:48:10Z

I think this small PR is okay. But honestly, the user's OS is far-outdated, from 2009. We should not spend significant effort maintaining something so old.

@morrisonlevi This is not the issue. We have gcc15 (current master) working on this 2009 macOS, so there is no problem to support any C standard. But if C11 is not required, then the code is supposed to compile with C99.

Clang emits the best errors when violating the provided standard.

nielsdos · 2024-07-24T11:11:36Z

From a quick google it seems that C11 support was introduced in gcc 4.7, and "GCC 4.9 Changes: “ISO C11 support is now at a similar level of completeness to ISO C99 support". GCC 4.9 is from 2014 and GCC 4.7 is from 2012. So the compilers are 10-12 years old. Does it make sense to make these changes? You'd also have to make the changes on lower branches I guess...

iluuu1994 · 2024-07-24T11:13:56Z

You'd also have to make the changes on lower branches I guess...

I was assuming that lower branches compiled with no warnings ~~but it doesn't seem that's the case~~. Edit: Actually, there are no warnings for 8.2 and 8.3. So this is indeed a new issue for master.

Does it make sense to make these changes?

I guess not. I did not expect there to be this many issues. I will consult the mailing list about switching to C11.

barracuda156 · 2024-07-24T11:52:06Z

Anything which can build with gcc10 is fine, IMO, since gcc10 can be bootstrapped without support for C11/C11+ is the system compiler. So while one has to build a suitable compiler first, it is a one-step pathway (and honestly at least C11/C11+ is needed nowadays so often that hardly anyone actually using legacy systems is limited to gcc 4.x). Requiring something newer than gcc10 can build is a bit problematic, though even that is not really a stopper.

iluuu1994 · 2024-07-24T16:15:03Z

@cmb69 It seems like MSVC only supports C11 from Visual Studio 2019 [1] but VS 2015 and 2017 remain supported [2]. Do you think that is a problem?

[1] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/
[2] https://learn.microsoft.com/en-us/visualstudio/productinfo/vs-servicing#older-versions-of-visual-studio

cmb69 · 2024-07-24T17:19:57Z

@iluuu1994, no problem there since VisualStudio 2017 is no longer used for PHP 8 officially. Some users might still use it, but I don't think that should be of concern. And particularly about the typedef redefinition, I don't think that's an issue even for older Visual Studio, since that is allowed in C++ anyway (and VS doesn't make a big difference between C and C++).

Anyway, I think requiring C11 should be discussed on the mailing list; hopefully a quick and uncontroversal discussion not requiring an RFC.

iluuu1994 · 2024-07-24T17:22:18Z

@cmb69 I agree. I was just gathering information about common environments to send an informed e-mail. Thank you!

petk · 2024-07-24T18:01:02Z

@iluuu1994 can we also set a minimum GCC and Clang compilers versions while bumping the C11? Because there is this constant issue of what is the minimum GCC version supported in PHP. I know that this might not be such a common practice out there but it will be quite helpful. Getting version in Autoconf can be done something like this: https://github.com/autoconf-archive/autoconf-archive/blob/master/m4/ax_compiler_version.m4 but this will be adjusted to a bit simpler PHP_COMPILER or something M4 macro.

bwoebi · 2024-07-24T18:13:09Z

Zend/zend_builtin_functions.h

@@ -22,12 +22,12 @@

 #include "zend_types.h"

-typedef struct _zval_struct zval;
+struct _zval_struct;


Stupid question maybe, but why do we need this at all? The typedef is defined in zend_types.h which is included right above?

Ah, this one is indeed probably unneeded.

bwoebi · 2024-07-24T18:16:16Z

@iluuu1994 I think there are quite a few places in Zend/ (e.g. zend_frameless_functions.h) which reference definitions from zend_string.h or zend_types.h. I think there's no need to avoid including zend_types.h/zend_string.h multiple times. These files are anyway relatively small. and will sort of anyway be somehow included by any file using Zend things.

ryandesign · 2024-07-24T18:26:17Z

I'm always baffled when I encounter these redefinition of typedef errors. Why are programmers so eager to redefine typedefs? Isn't it more confusing when a typedef is one thing in one place in the code and another thing somewhere else? Programmers got along fine with having a single meaning for a given typedef before C11 made redefinitions possible.

can we also set a minimum GCC and Clang compilers versions while bumping the C11?

Why?

Requiring a C99 compiler, or a C11 compiler if you absolutely must, should be sufficient.

If it isn't, and you want to define lower limits for clang, do not forget that there are two different compilers called clang—one released by llvm.org and one released by Apple—and they use different version numbering schemes, so you must state what the lower limit is for each of them.

nielsdos · 2024-07-24T18:49:21Z

For DOM, because I don't want to include another header inside the header file to get a certain typedef.

But I'm always baffled when people pull out ancient compiler versions 🤷.

iluuu1994 · 2024-07-24T19:05:25Z

I agree. Some of the PHP headers are gigantic and it's easy to run into recursive dependencies when not careful. I added some new headers that are included in common headers, and to avoid adding more dependencies I used some forward declarations.

petk · 2024-07-24T19:19:03Z

Why bumping compiler minimum version? Because of the compiler features that we constantly check here and there. For example of issues like this #14497 and for example checking the attribute is also inconsistent in php-src at the moment.

There are still some checks for GCC version 2 in the code. Obviously PHP doesn't build on GCC 2 anymore.

Treating compiler versions like part of the requirements. Minimum re2c, bison, Autoconf, and also build tools like compiler sounds quite logical to me in the very inconsistent C landscape.

petk · 2024-07-24T20:13:39Z

do not forget that there are two different compilers called clang—one released by llvm.org and one released by Apple—and they use different version numbering schemes

And yes, I agree. The "AppleClang" should be treated as a standalone compiler here indeed. So, checking for version of AppleClang separately from Clang. This Clang isn't so much of an issue actually. This would be an ideal case or in the bare minimum to check for GCC version.

bukka · 2024-07-24T23:02:47Z

I had recently a bug report (by email as that person did not have GitHub) running GCC 4.9. I doubt that there will be too many older versions in use so if we can set minimal version to 4.9, it should be hopefully fine even for 8.4. We could consider further bump in the future but 4.9 would be probably good start IMHO.

petk · 2024-07-25T23:19:30Z

Just adding here another example of the compiler version issue: https://bugs.php.net/bug.php?id=77883

Probably, I've also missed something, because I haven't finished reviewing entire php-src attributes and similar compiler features implementation but I also still think that setting only the C standard wouldn't be enough in PHP's case. For example, should the attributes be handled in the C code conditionally etc. without limiting the minimum compiler version(s) or should some minimum compiler version be a requirement...

cmb69 · 2024-07-31T18:48:30Z

@iluuu1994, are you still planning to write to internals? I don't want to rush, but I think this issue should be resolved prior to 8.4.0beta1. If you don't have time, I can send the mail instead (although I would be happy if you could provide me with info about compiler compatibility you have gathered so far).

Treating compiler versions like part of the requirements. Minimum re2c, bison, Autoconf, and also build tools like compiler sounds quite logical to me in the very inconsistent C landscape.

I'd rather focus on a C (C++) standard, and would only document exact versions of the most important compilers (and even that probably only if there's actual need, i.e. if we come across an issue with an old compiler version). I can imagine there are still a couple of compilers (or variants) in use, we don't really know about.

iluuu1994 · 2024-07-31T18:50:44Z

@cmb69 Yes, sorry for the delay. I'll try to send it tonight or tomorrow.

iluuu1994 · 2024-08-01T14:22:36Z

Hi everyone

We've gotten a bug report about compile errors in the PHP 8.4 alpha on old C99 compilers (and on some modern compilers when passing the -std=c99 flag). Specifically, C99 does not support typedef redeclarations, which is a C11 feature.

// some_header.h
typedef struct _zval_struct zval;
void some_func(zval *zv);

// zend_types.h
struct _zval_struct { ... };
typedef struct _zval_struct zval;

Some headers might want to forward declare zval so that zend_types.h doesn't have to be included. The two primary reasons you might want to avoid that is 1. to reduce compile times if the header is very large and 2. to prevent recursive header dependencies.

However, in C99 this code is actually illegal if both of these headers end up being included, because redeclarations of typedefs are not allowed. To fix it, the typedef must be removed and the signatures must refer to the struct directly.

// some_header.h
struct _zval_struct;
void some_func(struct _zval_struct *zv);

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] claim 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.

GCC gained support for C11 in 4.7 [4], Clang in 3.1, both released in 2012. For context, Debian Bullseye comes with GCC 10.2 [5], Ubuntu Focal Fossa comes with GCC 9.3 [6], RHEL 8 comes with GCC 8.x [7]. The biggest blocker in the past was MSVC, which has finally added C11 support in Visual Studio 2019 [8]. Technically, Visual Studio 2015 and 2017 are still supported by Microsoft [9], but according to Christoph they are no longer used for PHP builds since version 8.

Hence, it seems like it would be ok to bump our C compiler requirement to C11. We'd like to make this change before beta 1 if there are no objections. If you have any, please let me know.

Ilija

[1] #15079
[2]

php-src/CODING_STANDARDS.md

Line 12 in 8b6f14a

1. PHP is implemented in C99.

[3] https://www.php.net/manual/en/install.unix.php
[4] https://stackoverflow.com/a/16256596
[5] https://packages.debian.org/bullseye/gcc
[6] https://packages.ubuntu.com/focal/gcc
[7] https://access.redhat.com/solutions/19458
[8] https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/
[9] https://learn.microsoft.com/en-us/visualstudio/productinfo/vs-servicing

Sounds ok?

cmb69 · 2024-08-01T14:39:35Z

@iluuu1994, looks very good. Maybe add

that there are no plans to use any other new features of C11 for 8.4
that switching to C11 also solves the issue with VLAs which are mandatory in C99 but optional in C11, and not supported by MSVC at all

ryandesign · 2024-08-01T19:28:31Z

GCC gained support for C11 in 4.7 [4], Clang in 3.1, both released in 2012.

As I mentioned previously, there are two different things called clang so it is importantly to specify which one one is talking about: llvm.org clang or Apple clang. According to this table, Apple clang 3.1, 4.0, and 4.1 were all based on prerelease versions of llvm.org clang 3.1 and Apple clang 4.2 was based on a prerelease version of llvm.org clang 3.2. If the feature you need was introduced in llvm.org clang 3.1, I do not know in which version of Apple clang it was introduced.

thg2k · 2024-08-02T13:27:12Z

ext/random/php_random.h

-typedef struct _php_random_fallback_seed_state php_random_fallback_seed_state;
+struct _php_random_fallback_seed_state;


You can keep this as is, just remove the typedef struct.. later in the same file.

iluuu1994 · 2024-08-23T14:31:48Z

Closing in favor of #15397. For context: https://externals.io/message/124706#124922

Avoid C99 typedef redeclaration error

fd04377

Fixes phpGH-15070

iluuu1994 requested a review from cmb69 July 23, 2024 16:10

github-actions bot added the Category: Engine label Jul 23, 2024

cmb69 reviewed Jul 23, 2024

View reviewed changes

cmb69 approved these changes Jul 23, 2024

View reviewed changes

TimWolla reviewed Jul 23, 2024

View reviewed changes

iluuu1994 added 2 commits July 24, 2024 12:47

Fix C99 typedef redeclaration errors in Zend/

8c52e7d

Fix C11 asm keyword

bb878f9

C99 uses __asm__

iluuu1994 added 4 commits July 24, 2024 12:48

Fix C99 typedef redeclaration errors in ext-random

ca2061b

Fix C99 typedef redeclaration errors in opcache

939b283

Fix C99 typedef redeclaration errors in ext-dom

3516395

Compile with -std=c99 in CI with Clang

5d22529

Clang emits the best errors when violating the provided standard.

iluuu1994 requested review from dstogov, bukka, zeriyoshi and nielsdos as code owners July 24, 2024 10:51

github-actions bot added Extension: dom Extension: opcache labels Jul 24, 2024

Fix ext-bcmath c99 typedef redeclaration errors

e44882e

github-actions bot added the Extension: bcmath label Jul 24, 2024

Fix one more header

13887bd

Fix gdb source file

e94ce69

github-actions bot added the Category: Build System label Jul 24, 2024

bwoebi reviewed Jul 24, 2024

View reviewed changes

thg2k mentioned this pull request Aug 2, 2024

Avoid C99 typedef redeclaration error #15202

Draft

thg2k reviewed Aug 2, 2024

View reviewed changes

iluuu1994 closed this Aug 23, 2024

		typedef struct _php_random_fallback_seed_state php_random_fallback_seed_state;
		struct _php_random_fallback_seed_state;

Avoid C99 typedef redeclaration error #15079

Avoid C99 typedef redeclaration error #15079

Conversation

iluuu1994 commented Jul 23, 2024

petk commented Jul 23, 2024

iluuu1994 commented Jul 23, 2024

cmb69 commented Jul 23, 2024

iluuu1994 commented Jul 23, 2024

cmb69 left a comment

Choose a reason for hiding this comment

cmb69 commented Jul 23, 2024

iluuu1994 commented Jul 23, 2024 • edited Loading

morrisonlevi commented Jul 23, 2024

TimWolla Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

cmb69 Jul 23, 2024

Choose a reason for hiding this comment

cmb69 commented Jul 23, 2024

iluuu1994 commented Jul 23, 2024 • edited Loading

barracuda156 commented Jul 24, 2024

iluuu1994 commented Jul 24, 2024

barracuda156 commented Jul 24, 2024

nielsdos commented Jul 24, 2024

iluuu1994 commented Jul 24, 2024 • edited Loading

barracuda156 commented Jul 24, 2024

iluuu1994 commented Jul 24, 2024

cmb69 commented Jul 24, 2024

iluuu1994 commented Jul 24, 2024 • edited Loading

petk commented Jul 24, 2024

bwoebi Jul 24, 2024

Choose a reason for hiding this comment

iluuu1994 Jul 24, 2024

Choose a reason for hiding this comment

bwoebi commented Jul 24, 2024 • edited Loading

ryandesign commented Jul 24, 2024

nielsdos commented Jul 24, 2024

iluuu1994 commented Jul 24, 2024

petk commented Jul 24, 2024 • edited Loading

petk commented Jul 24, 2024

bukka commented Jul 24, 2024

petk commented Jul 25, 2024

cmb69 commented Jul 31, 2024

iluuu1994 commented Jul 31, 2024

iluuu1994 commented Aug 1, 2024 • edited Loading

cmb69 commented Aug 1, 2024

ryandesign commented Aug 1, 2024

thg2k Aug 2, 2024

Choose a reason for hiding this comment

iluuu1994 commented Aug 23, 2024

iluuu1994 commented Jul 23, 2024 •

edited

Loading

TimWolla Jul 23, 2024 •

edited

Loading

iluuu1994 commented Jul 23, 2024 •

edited

Loading

iluuu1994 commented Jul 24, 2024 •

edited

Loading

iluuu1994 commented Jul 24, 2024 •

edited

Loading

bwoebi commented Jul 24, 2024 •

edited

Loading

petk commented Jul 24, 2024 •

edited

Loading

iluuu1994 commented Aug 1, 2024 •

edited

Loading