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

Build system updates for modern C #10751

Closed
orlitzky opened this issue Mar 2, 2023 · 9 comments · Fixed by #10785
Closed

Build system updates for modern C #10751

orlitzky opened this issue Mar 2, 2023 · 9 comments · Fixed by #10785

Comments

@orlitzky
Copy link
Contributor

orlitzky commented Mar 2, 2023

Description

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

clang-16 (imminent) and probably gcc-14 (next yearish) are going to turn some warnings into errors:

  • -Werror=implicit-function-declaration
  • -Werror=implicit-int
  • -Werror=int-conversion (this is in Clang 15, actually)
  • -Werror=incompatible-function-pointer-types (GCC does not have a specific equivalent error, use -Werror=incompatible-pointer-types instead when testing)

Issues in the PHP source itself will probably be caught by developers, but issues buried in the build system are more subtle. For example, in ext/iconv/config.m4,

    if test -z "$iconv_impl_name"; then
      AC_MSG_CHECKING([if using GNU libiconv])
      AC_RUN_IFELSE([AC_LANG_SOURCE([[
#include <iconv.h>                                                                                                                                                           
int main() {
  printf("%d", _libiconv_version);
  return 0;
}
      ]])],[
        AC_MSG_RESULT(yes)
        iconv_impl_name="gnu_libiconv"
      ],[
        AC_MSG_RESULT(no)
      ],[
        AC_MSG_RESULT([no, cross-compiling])
      ])
    fi

That feature test is missing #include <stdio.h>, so printf() is undefined when it is used. Thus the feature test can fail with clang-16 when it should pass.

The other issue is visible above is,

int main() { ... }

That needs the correct signature,

int main(int argc, char** argv) { ... }

The same fix is needed in a few other m4 files, but is relatively easy to grep for.

PHP Version

8.2.3

Operating System

No response

@mvorisek
Copy link
Contributor

mvorisek commented Mar 2, 2023

maybe we want to assert some configure checks using Github CI?

@Girgias
Copy link
Member

Girgias commented Mar 3, 2023

If those compiler flags are enabled via CFLAGS= would this trigger the warnings or not? Could you also provide a PR (if not already done so)?

@orlitzky
Copy link
Contributor Author

orlitzky commented Mar 3, 2023

Yes, adding those flags to $CFLAGS is usually enough to reproduce the problem.

I can handle the PR. I would have just done that in the first place but I've been swamped this week and didn't want to forget about it.

A CI check might be a good idea. This was caught by a CI check on Gentoo. However that check also turned up a handful of false positives, where a function is "implicitly declared" in the ./configure test because it doesn't actually exist in the header. Which is of course expected if you're looking inside some header for a function and it isn't there. That was for -Werror=implicit-function-declaration, but the other errors/warnings might be more suitable for grepping without causing false positives; I'd have to think about it.

@orlitzky
Copy link
Contributor Author

orlitzky commented Mar 5, 2023

PR at #10785

Using gcc-12 with

CFLAGS="-O2 -pipe -march=native -Werror=implicit-function-declaration -Werror=implicit-int -Werror=strict-prototypes -Werror=incompatible-pointer-types -Werror=int-conversion"

grepping config.log (with the default build options) hits only -Wimplicit-function-declaration. Those look like false positives and I don't know what to do about them.

The others, you could probably just add a grep to the CI. If the warnings aren't triggered now, rejecting code that does trigger them can at worst make everybody stop and think. But I can't imagine (right now...) a reason why you would want to write standards non-compliant autoconf tests for anything other than implicit-function-definition.

@Girgias Girgias linked a pull request Mar 7, 2023 that will close this issue
Girgias pushed a commit that referenced this issue Mar 7, 2023
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
Girgias pushed a commit that referenced this issue Mar 7, 2023
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
Copy link
Member

Girgias commented Mar 7, 2023

Thank you!

@andypost
Copy link
Contributor

The issue incomplete, imap also needs work, just faced https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/45389#note_298190

@Girgias
Copy link
Member

Girgias commented Mar 26, 2023

Please open up a new issue

@andypost
Copy link
Contributor

Filed #10947

@andypost
Copy link
Contributor

Is it possible to backport to 8.2 branch?

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

Successfully merging a pull request may close this issue.

4 participants