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

Move iconv const check into autoconf #16847

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NattyNarwhal
Copy link
Member

Some systems (older NetBSD pre-10, Solaris) may have a non-standard const parameter with iconv. Instead of hardcoding the OS check, move this to a feature check performed by autoconf.

autoconf doesn't have a nicer way of checking this (well, except for AM_ICONV, which is part of gettext and we're presumably not using it), so we have to repeat the function signature and check for it with a mismatched signature.

Tested on NetBSD/amd64 9.4 (for const) and macOS/arm64 14 (for non-const).

Some systems (older NetBSD pre-10, Solaris) may have a non-standard
const parameter with iconv. Instead of hardcoding the OS check, move
this to a feature check performed by autoconf.

autoconf doesn't have a nicer way of checking this (well, except for
AM_ICONV, which is part of gettext and we're presumably not using it),
so we have to repeat the function signature and check for it with a
mismatched signature.
@NattyNarwhal
Copy link
Member Author

I added @devnexen and @petk for review since this basically obsoletes GH-16619.

@NattyNarwhal
Copy link
Member Author

I suspect the Windows failure is because AC_DEFINE in the Windows build system will expand an empty string into a "" even if quote is false:

        if (quote && typeof(value) == "string") {
                value = '"' + value.replace(new RegExp('(["\\\\])', "g"), '\\$1') + '"';
        } else if (typeof(value) != "undefined" && value.length == 0) {
                value = '""';
        }

@petk
Copy link
Member

petk commented Nov 20, 2024

I should check this compilation test in more details but something like this can be done here:

  • instead of defining a configuration preprocessor macro, a simple compile definition can be added to CFLAGS only in Autotools.
index 4d0dc36489..4aab9945ca 100644
--- a/ext/iconv/config.m4
+++ b/ext/iconv/config.m4
@@ -83,6 +83,18 @@ int main(void) {
   AS_VAR_IF([php_cv_iconv_errno], [yes],,
     [AC_MSG_FAILURE([The iconv check failed, 'errno' is missing.])])
 
+  dnl Some systems (older NetBSD pre-10, Solaris) may have a non-standard iconv
+  dnl signagure with const parameter. libiconv tends to match the system iconv
+  dnl too.
+  AC_CACHE_CHECK([if iconv has non-standard input parameter],
+    [php_cv_iconv_const],
+    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([
+      #include <iconv.h>
+      size_t iconv(iconv_t cd, const char **src, size_t *srcleft, char **dst, size_t *dstleft);])],
+    [php_cv_iconv_const=yes],
+    [php_cv_iconv_const=no])])
+  AS_VAR_IF([php_cv_iconv_const], [yes], [ICONV_CFLAGS="-DICONV_CONST=const"])
+
   AC_CACHE_CHECK([if iconv supports //IGNORE], [php_cv_iconv_ignore],
     [AC_RUN_IFELSE([AC_LANG_SOURCE([
 #include <iconv.h>
@@ -120,7 +132,7 @@ int main(void) {
   PHP_NEW_EXTENSION([iconv],
     [iconv.c],
     [$ext_shared],,
-    [-DZEND_ENABLE_STATIC_TSRMLS_CACHE=1])
+    [-DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 $ICONV_CFLAGS])
   PHP_SUBST([ICONV_SHARED_LIBADD])
   PHP_INSTALL_HEADERS([ext/iconv], [php_iconv.h])
 fi
diff --git a/ext/iconv/iconv.c b/ext/iconv/iconv.c
index 4241b7c288..e1cb5ef3f5 100644
--- a/ext/iconv/iconv.c
+++ b/ext/iconv/iconv.c
@@ -43,12 +43,8 @@
 #undef iconv
 #endif
 
-#if defined(__NetBSD__)
-// unfortunately, netbsd has still the old non posix conformant signature
-// libiconv tends to match the eventual system's iconv too.
-#define ICONV_CONST const
-#else
-#define ICONV_CONST
+#ifndef ICONV_CONST
+# define ICONV_CONST
 #endif
 
 #include "zend_smart_str.h"

Yes, that GNU lib's iconv check looks like some all-in-one solution with all edge cases considered but it would be also probably a bit over-engineered to add it into PHP: https://git.savannah.gnu.org/cgit/gnulib.git/tree/m4/iconv.m4

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.

2 participants