Skip to content
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

Fix GH-10751 #10785

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Fix GH-10751 #10785

merged 2 commits into from
Mar 7, 2023

Conversation

orlitzky
Copy link
Contributor

@orlitzky orlitzky commented Mar 5, 2023

Modernize some autoconf C code to avoid problems with the next generation of compilers.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me, although just having a C spec question.

build/php.m4 Outdated
int main(void)
int main(int argc, char** argv)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't int main(void) one of the valid signatures of the main function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it looks like you're right in the C23 draft. I'm happy to leave that line alone, or even to make them all int main(void).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changing them all to main(void) would be my preference. Also, I'm pretty sure this is part of the specification since forever (and definitely since C99). But good to know C23 doesn't change this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worded that rather badly. I just meant that the freely available C23 draft was what was easiest for me to check.

I've updated them all to int main(void).

The next generation of C compilers is going to enforce the C standard
more strictly:

  https://wiki.gentoo.org/wiki/Modern_C_porting

One warning that will soon become an error is -Wstrict-prototypes.
This is relatively easy to catch in most code (it will fail to
compile), but inside of autoconf tests it can go unnoticed because
many feature-test compilations fail by design. For example,

  $ export CFLAGS="$CFLAGS -Werror=strict-prototypes"
  $ ./configure
  ...
  checking if iconv supports errno... no
  configure: error: iconv does not support errno

(this is on a system where iconv *does* support errno). If errno
support were optional, that test would have "silently" disabled
it. The underlying issue here, from config.log, is

  conftest.c:211:5: error: function declaration isn't a prototype
  [-Werror=strict-prototypes]
    211 | int main() {

This commit goes through all of our autoconf tests, replacing main()
with main(void). Up to equivalent types and variable renamings, that's
one of the two valid signatures, and satisfies the compiler (gcc-12 in
this case).

Fixes GH-10751
The next generation of C compilers is going to enforce the C standard
more strictly:

  https://wiki.gentoo.org/wiki/Modern_C_porting

One warning that will eventually become an error is
-Wimplicit-function-declaration. This is relatively easy to catch in
most code (it will fail to compile), but inside of autoconf tests it
can go unnoticed because many feature-test compilations fail by
design. For example,

  AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <iconv.h>]],
                 [[iconv_ccs_init(NULL, NULL);]])]...

is designed to fail if iconv_ccs_init() is not in iconv.h. On the
other hand,

  AC_RUN_IFELSE([AC_LANG_SOURCE([[
  #include <iconv.h>
  int main() {
    printf("%d", _libiconv_version);
    return 0;
  }

should pass if _libiconv_version is defined. If the user has
-Werror=implicit-function-declaration in his CFLAGS, however,
it will not:

  $ export CFLAGS="$CFLAGS -Werror=implicit-function-declaration"
  $ ./configure
  ...
  checking if using GNU libiconv... no

This is because the stdio.h header that defines printf() is missing:

  conftest.c:240:3: error: implicit declaration of function 'printf'
  [-Werror=implicit-function-declaration]
    240 |   printf("%d", _libiconv_version);
        |   ^~~~~~
  conftest.c:239:1: note: include '<stdio.h>' or provide a declaration
  of 'printf'

This commit adds the include, correcting the test with any compiler
that balks at implicit function definitions.

Closes GH-10751
@Girgias Girgias linked an issue Mar 7, 2023 that may be closed by this pull request
@Girgias Girgias merged commit 88b30e0 into php:master Mar 7, 2023
@nielsdos
Copy link
Member

@Girgias Shouldn't this be backported to 8.1 & 8.2 too? Or am I missing something?
New compiler versions shouldn't break the build of older PHP versions that are still supported.

@Girgias
Copy link
Member

Girgias commented Mar 26, 2023

@Girgias Shouldn't this be backported to 8.1 & 8.2 too? Or am I missing something? New compiler versions shouldn't break the build of older PHP versions that are still supported.

Probably? Usually we don't backport fixes for new compiler warnings. But this does break building it so... would make sense.

@nielsdos
Copy link
Member

Yeah, especially with Clang 16 and the recent issue report I think we need to.
Do you want to backport it or should I do it?

@orlitzky
Copy link
Contributor Author

Thanks guys. I just opened #10948 which will be even more important to backport since it disables the imap extension with clang-16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build system updates for modern C
3 participants