Thread: [PATCH v2 1/1] Fix detection of pwritev support for OSX.

Fixes:
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -O2
-I../../../../src/include -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk   -c -o fd.o fd.c
 
fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
                part = pg_pwritev(fd, iov, iovcnt, offset);
                       ^~~~~~~~~~
../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
                   ^~~~~~~

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk/usr/include/sys/uio.h:104:9:
note:'pwritev' has been marked as being introduced in macOS 11.0
 
      here, but the deployment target is macOS 10.15.0
ssize_t pwritev(int, const struct iovec *, int, off_t) __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0),
ios(14.0),watchos(7.0), tvos(14.0));
 
        ^
fd.c:3661:10: note: enclose 'pwritev' in a __builtin_available check to silence this warning
                part = pg_pwritev(fd, iov, iovcnt, offset);
                       ^~~~~~~~~~
../../../../src/include/port/pg_iovec.h:49:20: note: expanded from macro 'pg_pwritev'
                   ^~~~~~~
1 warning generated.

This results in a runtime error:
running bootstrap script ... dyld: lazy symbol binding failed: Symbol not found: _pwritev
  Referenced from: /usr/local/pgsql/bin/postgres
  Expected in: /usr/lib/libSystem.B.dylib

dyld: Symbol not found: _pwritev
  Referenced from: /usr/local/pgsql/bin/postgres
  Expected in: /usr/lib/libSystem.B.dylib

child process was terminated by signal 6: Abort trap: 6

To fix this we set -Werror=unguarded-availability-new so that a compile
test for pwritev will fail if the symbol is unavailable on the requested
SDK version.
---
Changes v1 -> v2:
  - Add AC_LIBOBJ(pwritev) when pwritev not available
  - set -Werror=unguarded-availability-new for CXX flags as well
---
 configure    | 145 ++++++++++++++++++++++++++++++++++++++++++++++-----
 configure.ac |  21 +++++++-
 2 files changed, 152 insertions(+), 14 deletions(-)

diff --git a/configure b/configure
index 8af4b99021..662b0ae9ce 100755
--- a/configure
+++ b/configure
@@ -5373,6 +5373,98 @@ if test x"$pgac_cv_prog_CC_cflags__Werror_vla" = x"yes"; then
 fi
 
 
+  # Prevent usage of symbols marked as newer than our target.
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Werror=unguarded-availability-new, for
CFLAGS">&5
 
+$as_echo_n "checking whether ${CC} supports -Werror=unguarded-availability-new, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=yes
+else
+  pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Werror_unguarded_availability_new" = x"yes"; then
+  CFLAGS="${CFLAGS} -Werror=unguarded-availability-new"
+fi
+
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -Werror=unguarded-availability-new, for
CXXFLAGS">&5
 
+$as_echo_n "checking whether ${CXX} supports -Werror=unguarded-availability-new, for CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=yes
+else
+  pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__Werror_unguarded_availability_new" = x"yes"; then
+  CXXFLAGS="${CXXFLAGS} -Werror=unguarded-availability-new"
+fi
+
+
   # -Wvla is not applicable for C++
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wendif-labels, for CFLAGS" >&5
@@ -15715,6 +15807,46 @@ $as_echo "#define HAVE_PS_STRINGS 1" >>confdefs.h
 
 fi
 
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for pwritev" >&5
+$as_echo_n "checking for pwritev... " >&6; }
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#ifdef HAVE_SYS_UIO_H
+#include <sys/uio.h>
+#endif
+int
+main ()
+{
+struct iovec *iov;
+off_t offset;
+offset = 0;
+pwritev(0, iov, 0, offset);
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+
+$as_echo "#define HAVE_PWRITEV 1" >>confdefs.h
+
+else
+  { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
+$as_echo "no" >&6; }
+case " $LIBOBJS " in
+  *" pwritev.$ac_objext "* ) ;;
+  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
+ ;;
+esac
+
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+
 ac_fn_c_check_func "$LINENO" "dlopen" "ac_cv_func_dlopen"
 if test "x$ac_cv_func_dlopen" = xyes; then :
   $as_echo "#define HAVE_DLOPEN 1" >>confdefs.h
@@ -15871,19 +16003,6 @@ esac
 
 fi
 
-ac_fn_c_check_func "$LINENO" "pwritev" "ac_cv_func_pwritev"
-if test "x$ac_cv_func_pwritev" = xyes; then :
-  $as_echo "#define HAVE_PWRITEV 1" >>confdefs.h
-
-else
-  case " $LIBOBJS " in
-  *" pwritev.$ac_objext "* ) ;;
-  *) LIBOBJS="$LIBOBJS pwritev.$ac_objext"
- ;;
-esac
-
-fi
-
 ac_fn_c_check_func "$LINENO" "random" "ac_cv_func_random"
 if test "x$ac_cv_func_random" = xyes; then :
   $as_echo "#define HAVE_RANDOM 1" >>confdefs.h
diff --git a/configure.ac b/configure.ac
index 868a94c9ba..724881a7f0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -494,6 +494,9 @@ if test "$GCC" = yes -a "$ICC" = no; then
   AC_SUBST(PERMIT_DECLARATION_AFTER_STATEMENT)
   # Really don't want VLAs to be used in our dialect of C
   PGAC_PROG_CC_CFLAGS_OPT([-Werror=vla])
+  # Prevent usage of symbols marked as newer than our target.
+  PGAC_PROG_CC_CFLAGS_OPT([-Werror=unguarded-availability-new])
+  PGAC_PROG_CXX_CFLAGS_OPT([-Werror=unguarded-availability-new])
   # -Wvla is not applicable for C++
   PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
   PGAC_PROG_CXX_CFLAGS_OPT([-Wendif-labels])
@@ -1726,6 +1729,23 @@ if test "$pgac_cv_var_PS_STRINGS" = yes ; then
   AC_DEFINE([HAVE_PS_STRINGS], 1, [Define to 1 if the PS_STRINGS thing exists.])
 fi
 
+AC_MSG_CHECKING([for pwritev])
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
+[#ifdef HAVE_SYS_TYPES_H
+#include <sys/types.h>
+#endif
+#ifdef HAVE_SYS_UIO_H
+#include <sys/uio.h>
+#endif],
+[struct iovec *iov;
+off_t offset;
+offset = 0;
+pwritev(0, iov, 0, offset);
+])], [AC_MSG_RESULT(yes)
+AC_DEFINE([HAVE_PWRITEV], 1, [Define to 1 if you have the `pwritev' function.])],
+[AC_MSG_RESULT(no)
+AC_LIBOBJ(pwritev)])
+
 AC_REPLACE_FUNCS(m4_normalize([
     dlopen
     explicit_bzero
@@ -1739,7 +1759,6 @@ AC_REPLACE_FUNCS(m4_normalize([
     pread
     preadv
     pwrite
-    pwritev
     random
     srandom
     strlcat
-- 
2.30.0

James Hilliard <james.hilliard1@gmail.com> writes:
> Fixes:
> fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]

It's still missing preadv, and it still has nonzero chance of breaking
successful detection of pwritev on platforms other than yours, and it's
still really ugly.

But the main reason I don't want to go this way is that I don't think
it'll stop with preadv/pwritev.  If we make it our job to build
successfully even when using the wrong SDK version for the target
platform, we're going to be in for more and more pain with other
kernel APIs.

We could, of course, do what Apple wants us to do and try to build
executables that work across versions.  I do not intend to put up
with the sort of invasive, error-prone source-code-level runtime test
they recommend ... but given that there is weak linking involved here,
I wonder if there is a way to silently sub in src/port/pwritev.c
when executing on a pre-11 macOS, by dint of marking it a weak
symbol?

            regards, tom lane

On Tue, Jan 19, 2021 at 10:29 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> James Hilliard <james.hilliard1@gmail.com> writes:
> > Fixes:
> > fd.c:3661:10: warning: 'pwritev' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
>
> It's still missing preadv, and it still has nonzero chance of breaking
> successful detection of pwritev on platforms other than yours, and it's
> still really ugly.
Setting -Werror=unguarded-availability-new should in theory always
ensure that configure checks fail if the symbol is unavailable or marked
as requiring a target newer than the MACOSX_DEPLOYMENT_TARGET.
>
> But the main reason I don't want to go this way is that I don't think
> it'll stop with preadv/pwritev.  If we make it our job to build
> successfully even when using the wrong SDK version for the target
> platform, we're going to be in for more and more pain with other
> kernel APIs.
This issue really has nothing to do with the SDK version at all, it's the
MACOSX_DEPLOYMENT_TARGET that matters which must be taken
into account during configure in some way, this is what my patch does
by triggering the pwritev compile test error by setting
-Werror=unguarded-availability-new.

It's expected that MACOSX_DEPLOYMENT_TARGET=10.15 with a
MacOSX11.1.sdk will produce a binary that can run on OSX 10.15.

The MacOSX11.1.sdk is not the wrong SDK for a 10.15 target and
is fully capable of producing 10.15 compatible binaries.
>
> We could, of course, do what Apple wants us to do and try to build
> executables that work across versions.  I do not intend to put up
> with the sort of invasive, error-prone source-code-level runtime test
> they recommend ... but given that there is weak linking involved here,
> I wonder if there is a way to silently sub in src/port/pwritev.c
> when executing on a pre-11 macOS, by dint of marking it a weak
> symbol?
The check I added is strictly a compile time check still, not runtime.

I also don't think this is a weak symbol.

From the header file it is not have __attribute__((weak_import)):
ssize_t pwritev(int, const struct iovec *, int, off_t)
__DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0), ios(14.0),
watchos(7.0), tvos(14.0));
>
>                         regards, tom lane

James Hilliard <james.hilliard1@gmail.com> writes:
> I also don't think this is a weak symbol.

> From the header file it is not have __attribute__((weak_import)):
> ssize_t pwritev(int, const struct iovec *, int, off_t)
> __DARWIN_NOCANCEL(pwritev) __API_AVAILABLE(macos(11.0), ios(14.0),
> watchos(7.0), tvos(14.0));

See the other thread.  I found by looking at the asm output that
what __API_AVAILABLE actually does is cause the compiler to emit
a ".weak_reference" directive when calling a function it thinks
might not be available.  So there's some sort of weak linking
going on, though it's certainly possible that it's not shaped
in a way that'd help us do this the way we'd prefer.

            regards, tom lane