Follow up to last post

Follow up to last post

[Po Lu]

Okay, the problem seems to be a little more involved than I initially
thought.

Apparently, it is not ok to use gl_CHECK_FUNCS_ANDROID without removing
utimensat from AC_CHECK_FUNCS_ONCE, because the latter will define
HAVE_UTIMENSAT, leading to a build failure later (I only found this
after running `make maintainer-clean' in lib.)

diff --git a/m4/unistd_h.m4 b/m4/unistd_h.m4
index f4384027e37..dd799ae27db 100644
--- a/m4/unistd_h.m4
+++ b/m4/unistd_h.m4
@@ -50,6 +50,7 @@ AC_DEFUN_ONCE
     group_member isatty lchown link linkat lseek pipe pipe2 pread pwrite
     readlink readlinkat rmdir sethostname sleep symlink symlinkat
     truncate ttyname_r unlink unlinkat usleep])
+  gl_CHECK_FUNCS_ANDROID([ftruncate], [[#include <unistd.h>]])
 
   AC_REQUIRE([AC_C_RESTRICT])
 
diff --git a/m4/utimens.m4 b/m4/utimens.m4
index c5d9b69e6f5..2b87f0149b5 100644
--- a/m4/utimens.m4
+++ b/m4/utimens.m4
@@ -11,7 +11,8 @@ AC_DEFUN
   AC_REQUIRE([gl_FUNC_UTIMES])
   AC_REQUIRE([gl_CHECK_TYPE_STRUCT_TIMESPEC])
   AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
-  AC_CHECK_FUNCS_ONCE([futimens utimensat lutimes])
+  AC_CHECK_FUNCS_ONCE([futimens lutimes])
+  gl_CHECK_FUNCS_ANDROID([utimensat], [[#include <sys/stat.h>]])
   gl_CHECK_FUNCS_ANDROID([futimes], [[#include <sys/time.h>]])
   gl_CHECK_FUNCS_ANDROID([futimesat], [[#include <sys/time.h>]])
 
diff --git a/m4/utimensat.m4 b/m4/utimensat.m4
index dd210fc989a..8edea60bd10 100644
--- a/m4/utimensat.m4
+++ b/m4/utimensat.m4
@@ -13,9 +13,12 @@ AC_DEFUN
   AC_REQUIRE([gl_SYS_STAT_H_DEFAULTS])
   AC_REQUIRE([gl_USE_SYSTEM_EXTENSIONS])
   AC_REQUIRE([AC_CANONICAL_HOST]) dnl for cross-compiles
-  AC_CHECK_FUNCS_ONCE([utimensat])
+  # This is necessary for cross-compiles, because otherwise utimensat
+  # will appear to work.
+  gl_CHECK_FUNCS_ANDROID([utimensat], [[#include <sys/stat.h>]])
   if test $ac_cv_func_utimensat = no; then
     HAVE_UTIMENSAT=0
+    REPLACE_UTIMENSAT=1
   else
     AC_CACHE_CHECK([whether utimensat works],
       [gl_cv_func_utimensat_works],

Re: Follow up to last post

[Paul Eggert]

On 1/18/23 05:53, Po Lu wrote:
> +  gl_CHECK_FUNCS_ANDROID([utimensat], [[#include <sys/stat.h>]])
>     if test $ac_cv_func_utimensat = no; then
>       HAVE_UTIMENSAT=0
> +    REPLACE_UTIMENSAT=1

REPLACE_UTIMENSAT=1 means utimensat is present but broken and needs to 
be replaced. However, $ac_cv_func_utimensat = no means utimensat is 
absent. So I'm puzzled as to why REPLACE_UTIMENSAT=1 would be 
appropriate here.

Re: API levels and functions on Android

[Bruno Haible]

Paul Eggert wrote:
> > +  gl_CHECK_FUNCS_ANDROID([utimensat], [[#include <sys/stat.h>]])
> >     if test $ac_cv_func_utimensat = no; then
> >       HAVE_UTIMENSAT=0
> > +    REPLACE_UTIMENSAT=1
> 
> REPLACE_UTIMENSAT=1 means utimensat is present but broken and needs to 
> be replaced. However, $ac_cv_func_utimensat = no means utimensat is 
> absent. So I'm puzzled as to why REPLACE_UTIMENSAT=1 would be 
> appropriate here.

Deployment on Android is a bit special: When a developer compiles an app,
they specify an "API level". This API level designates the minimum Android
version on which the app will work. For example [1][2]:

  18 = Android 4.3
  19 = Android 4.4
  20 = Android 4.4W
  21 = Android 5.0
  22 = Android 5.1
  23 = Android 6.0
  24 = Android 7.0
  25 = Android 7.1
  26 = Android 8.0
  27 = Android 8.1
  28 = Android 9
  29 = Android 10
  30 = Android 11
  31 = Android 12
  32 = Android 12L
  33 = Android 13

Now, some functions get added to libc over time. For example, the glob and
globfree functions are being added in API level 28. So, when a developer
*compiles* for an API level < 28 but tests the app on a device with API level
≥ 28, the two functions will not be declared in the header files but will be
present in libc.

By the way the gl_CHECK_FUNCS_ANDROID macro works so far, when a function is
not declared, it reports it as "missing" (ac_cv_func_...=no). This is a good
approximation, and will usually work fine.

But there is a conflict: Gnulib, when ac_cv_func_glob=no, will define the
function 'glob' (not 'rpl_glob'), since Gnulib so far assumes that a build
is for one OS version. So, we can arrive at a situation where
  - the OS defines 'glob' in libc.so, and
  - the app defines 'glob' in its shared library.
By ELF rules, the 'glob' symbol from the app's shared library will override
the one from libc. Which is OK for the app. But some calls from within libc
might behave unexpectedly if they suddenly call the 'glob' function from the
app.

Defining REPLACE_GLOB=1 despite HAVE_GLOB=0 will be the way to make Gnulib
define 'rpl_glob' instead of 'glob', so that there is no conflict with libc.so.

I still need to work out the details how this would/will work. So as to
do things correctly on Android, while OTOH not adding lots of configure tests
for the other platforms...

Bruno

[1] https://developer.android.com/studio/releases/platforms
[2] https://apilevels.com/

Re: API levels and functions on Android

[Paul Eggert]

On 1/18/23 15:23, Bruno Haible wrote:
> Defining REPLACE_GLOB=1 despite HAVE_GLOB=0 will be the way to make Gnulib
> define 'rpl_glob' instead of 'glob', so that there is no conflict with libc.so.
> 
> I still need to work out the details how this would/will work. So as to
> do things correctly on Android, while OTOH not adding lots of configure tests
> for the other platforms...

Thanks for looking into this.

On Android, should we always define REPLACE_WHATEVER when 'whatever' 
doesn't exist, at least for all the REPLACE_WHATEVER instances that 
currently exist in Gnulib? After all, 'whatever' might exist in some 
future Android version.

Would it make sense to wait until you've worked out the details, before 
installing patches like the REPLACE_UTIMENSAT=1 patch?

Re: API levels and functions on Android

[Bruno Haible]

Paul Eggert wrote:
> > Defining REPLACE_GLOB=1 despite HAVE_GLOB=0 will be the way to make Gnulib
> > define 'rpl_glob' instead of 'glob', so that there is no conflict with libc.so.
> > 
> > I still need to work out the details how this would/will work. So as to
> > do things correctly on Android, while OTOH not adding lots of configure tests
> > for the other platforms...
> 
> Thanks for looking into this.
> 
> On Android, should we always define REPLACE_WHATEVER when 'whatever' 
> doesn't exist, at least for all the REPLACE_WHATEVER instances that 
> currently exist in Gnulib? After all, 'whatever' might exist in some 
> future Android version.

I think this would be overkill. Remember, for debuggability, when Gnulib
defines a function 'foo', we want gdb to know it by the name 'foo', if
there is no imperative to name it differently.

The two known imperatives are
  - call it 'rpl_foo', to avoid a clash with libc,
  - when shipping a shared library libgaga, call it 'libgaga_foo' or 'gaga_foo',
    to avoid a clash with other shared libraries.
If none of these two imperatives apply, the name 'foo' without any prefix is
the natural choice.

> Would it make sense to wait until you've worked out the details, before 
> installing patches like the REPLACE_UTIMENSAT=1 patch?

Yes, definitely.

Bruno

Re: Follow up to last post

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 1/18/23 05:53, Po Lu wrote:
>> +  gl_CHECK_FUNCS_ANDROID([utimensat], [[#include <sys/stat.h>]])
>>     if test $ac_cv_func_utimensat = no; then
>>       HAVE_UTIMENSAT=0
>> +    REPLACE_UTIMENSAT=1
>
> REPLACE_UTIMENSAT=1 means utimensat is present but broken and needs to
> be replaced. However, $ac_cv_func_utimensat = no means utimensat is
> absent. So I'm puzzled as to why REPLACE_UTIMENSAT=1 would be
> appropriate here.

The problem is that utimensat *is* in the C library on the newer Android
systems, so when the binary runs on those, gnulib ends up replacing the
C library utimensat, with potentially horrid consequences.