Re: [PATCH] free: preserve errno

[Bruno Haible]

I wrote:
>   * lib/stdlib.in.h (free): New declaration.

Oops, I forgot to check this declaration in the test suite.


2020-12-18  Bruno Haible  

free-posix: Add C++ declaration test.
* tests/test-stdlib-c++.cc (free): New declaration.

diff --git a/tests/test-stdlib-c++.cc b/tests/test-stdlib-c++.cc
index 09bae83..5eb7299 100644
--- a/tests/test-stdlib-c++.cc
+++ b/tests/test-stdlib-c++.cc
@@ -47,6 +47,10 @@ SIGNATURE_CHECK (GNULIB_NAMESPACE::canonicalize_file_name, 
char *,
  (const char *));
 #endif
 
+#if GNULIB_TEST_FREE_POSIX
+SIGNATURE_CHECK (GNULIB_NAMESPACE::free, void, (void *));
+#endif
+
 #if GNULIB_TEST_GETLOADAVG
 SIGNATURE_CHECK (GNULIB_NAMESPACE::getloadavg, int, (double[], int));
 #endif

Re: [PATCH] free: preserve errno

[Paul Eggert]

Thanks for fixing that.

It is amusing that after 45 years we are still fussing with the semantics of 
'free'. You'd think it'd be simple, but no

Re: [PATCH] free: preserve errno

[Bruno Haible]

Pádraig Brady wrote:
> coreutils is now failing to build with:
> 
>lib/free.c:28:1: error: no previous declaration
>  for 'rpl_free' [-Werror=missing-declarations]
> 28 | rpl_free (void *p)
>| ^~~~
> 
> In early 2008 coreutils removed use of the then deprecated "free" module,
> but I now see 'canonicalize' explicitly depends on this now undeprecated 
> module.

What I see is a warning:

free.c:28:1: warning: no previous declaration for ‘rpl_free’ 
[-Wmissing-declarations]
 rpl_free (void *p)
 ^

Recall that Gnulib does not guarantee warning-free compilation on any
platform.

This patch should fix the warning, by using the modern idioms (that also support
C++ compilation). But there are more issues in this area; more patches to come.


2020-12-17  Bruno Haible  

free: Fix warning.
Reported by Pádraig Brady  in
.
* lib/stdlib.in.h (free): New declaration.
* m4/stdlib_h.m4 (gl_STDLIB_H): Test whether 'free' is declared.
(gl_STDLIB_H_DEFAULTS): Initialize GNULIB_FREE, REPLACE_FREE.
* modules/stdlib (Makefile.am): Substitute GNULIB_FREE, REPLACE_FREE.
* m4/free.m4 (gl_FUNC_FREE): Set REPLACE_FREE, instead of defining
'free' as a macro here.
* modules/free (Depends-on): Add stdlib.
(configure.ac): Test REPLACE_FREE. Invoke gl_STDLIB_MODULE_INDICATOR.

diff --git a/lib/stdlib.in.h b/lib/stdlib.in.h
index a76fbdc..a512459 100644
--- a/lib/stdlib.in.h
+++ b/lib/stdlib.in.h
@@ -284,6 +284,27 @@ _GL_CXXALIAS_SYS (fcvt, char *,
 _GL_CXXALIASWARN (fcvt);
 #endif
 
+#if @GNULIB_FREE@
+# if @REPLACE_FREE@
+#  if !(defined __cplusplus && defined GNULIB_NAMESPACE)
+#   undef free
+#   define free rpl_free
+#  endif
+_GL_FUNCDECL_RPL (free, void, (void *ptr));
+_GL_CXXALIAS_RPL (free, void, (void *ptr));
+# else
+_GL_CXXALIAS_SYS (free, void, (void *ptr));
+# endif
+# if __GLIBC__ >= 2
+_GL_CXXALIASWARN (free);
+# endif
+#elif defined GNULIB_POSIXCHECK
+# undef free
+/* Assume free is always declared.  */
+_GL_WARN_ON_USE (free, "free is not future POSIX compliant everywhere - "
+ "use gnulib module free for portability");
+#endif
+
 /* On native Windows, map 'gcvt' to '_gcvt', so that -loldnames is not
required.  In C++ with GNULIB_NAMESPACE, avoid differences between
platforms by defining GNULIB_NAMESPACE::gcvt on all platforms that have
diff --git a/m4/free.m4 b/m4/free.m4
index 62daa8e..9f108c1 100644
--- a/m4/free.m4
+++ b/m4/free.m4
@@ -1,5 +1,4 @@
-# Check whether free (NULL) is supposed to work.
-
+# free.m4 serial 1
 # Copyright (C) 2003-2005, 2009-2020 Free Software Foundation, Inc.
 # This file is free software; the Free Software Foundation
 # gives unlimited permission to copy and/or distribute it,
@@ -16,6 +15,7 @@
 
 AC_DEFUN([gl_FUNC_FREE],
 [
+  AC_REQUIRE([gl_STDLIB_H_DEFAULTS])
   AC_REQUIRE([AC_CANONICAL_HOST])
   AC_CACHE_CHECK([whether free (NULL) is known to work],
 [gl_cv_func_free],
@@ -67,10 +67,7 @@ AC_DEFUN([gl_FUNC_FREE],
 
   case $gl_cv_func_free,$gl_cv_func_free_preserves_errno in
*yes,*yes) ;;
-   *)
-AC_DEFINE([free], [rpl_free],
-  [Define to rpl_free if the replacement function should be used.])
-;;
+   *) REPLACE_FREE=1 ;;
   esac
 ])
 
diff --git a/m4/stdlib_h.m4 b/m4/stdlib_h.m4
index 8b3cfe3..6b519c4 100644
--- a/m4/stdlib_h.m4
+++ b/m4/stdlib_h.m4
@@ -1,4 +1,4 @@
-# stdlib_h.m4 serial 52
+# stdlib_h.m4 serial 53
 dnl Copyright (C) 2007-2020 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -22,7 +22,7 @@ AC_DEFUN([gl_STDLIB_H],
 #if HAVE_RANDOM_H
 # include 
 #endif
-]], [_Exit aligned_alloc atoll canonicalize_file_name
+]], [_Exit aligned_alloc atoll canonicalize_file_name free
 getloadavg getsubopt grantpt
 initstate initstate_r mbtowc mkdtemp mkostemp mkostemps mkstemp mkstemps
 posix_memalign posix_openpt ptsname ptsname_r qsort_r
@@ -62,6 +62,7 @@ AC_DEFUN([gl_STDLIB_H_DEFAULTS],
   GNULIB_ATOLL=0; AC_SUBST([GNULIB_ATOLL])
   GNULIB_CALLOC_POSIX=0;  AC_SUBST([GNULIB_CALLOC_POSIX])
   GNULIB_CANONICALIZE_FILE_NAME=0;  AC_SUBST([GNULIB_CANONICALIZE_FILE_NAME])
+  GNULIB_FREE=0;  AC_SUBST([GNULIB_FREE])
   GNULIB_GETLOADAVG=0;AC_SUBST([GNULIB_GETLOADAVG])
   GNULIB_GETSUBOPT=0; AC_SUBST([GNULIB_GETSUBOPT])
   GNULIB_GRANTPT=0;   AC_SUBST([GNULIB_GRANTPT])
@@ -140,6 +141,7 @@ AC_DEFUN([gl_STDLIB_H_DEFAULTS],
   REPLACE_ALIGNED_ALLOC=0;   AC_SUBST([REPLACE_ALIGNED_ALLOC])
   REPLACE_CALLOC=0;  AC_SUBST([REPLACE_CALLOC])
   REPLACE_CANONICALIZE_FILE_NAME=0;  AC_SUBST([REPLACE_CANONICALIZE_FILE_NAME])
+  REPLACE_FREE=0;AC_SUBST([REPLACE_FREE])
   REPLACE_INITSTATE=0;   AC_SUBST([REPLACE_INITSTATE])
   REPLACE_MALLOC=0;  AC_SUBST([REPLACE_MALLOC])
   REPLACE_MBTOWC=0;  

Re: [PATCH] free: preserve errno

[Pádraig Brady]

On 17/12/2020 09:54, Paul Eggert wrote:

* lib/free.c (rpl_free): Preserve errno.  Check for null only if
CANNOT_FREE_NULL is defined, as an optimization for POSIX 2008
platforms that do not preserve errno.
* m4/free.m4 (gl_FUNC_FREE): Check whether free preserves errno.
Also, define CANNOT_FREE_NULL if free cannot free NULL.
* modules/free (configure.ac): Also replace 'free' if
it does not preserve errno.


coreutils is now failing to build with:

  lib/free.c:28:1: error: no previous declaration
for 'rpl_free' [-Werror=missing-declarations]
   28 | rpl_free (void *p)
  | ^~~~

In early 2008 coreutils removed use of the then deprecated "free" module,
but I now see 'canonicalize' explicitly depends on this now undeprecated module.

I've worked around the issue,
but I'm not sure how you'd prefer to handle it in gnulib.

thanks,
Pádraig

[PATCH] free: preserve errno

[Paul Eggert]

* lib/free.c (rpl_free): Preserve errno.  Check for null only if
CANNOT_FREE_NULL is defined, as an optimization for POSIX 2008
platforms that do not preserve errno.
* m4/free.m4 (gl_FUNC_FREE): Check whether free preserves errno.
Also, define CANNOT_FREE_NULL if free cannot free NULL.
* modules/free (configure.ac): Also replace 'free' if
it does not preserve errno.
---
 ChangeLog|  9 +
 lib/free.c   | 12 ++--
 m4/free.m4   | 35 ++-
 modules/free | 12 ++--
 4 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 445e2c074..e97d82aa6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,14 @@
 2020-12-17  Paul Eggert  
 
+   free: preserve errno
+   * lib/free.c (rpl_free): Preserve errno.  Check for null only if
+   CANNOT_FREE_NULL is defined, as an optimization for POSIX 2008
+   platforms that do not preserve errno.
+   * m4/free.m4 (gl_FUNC_FREE): Check whether free preserves errno.
+   Also, define CANNOT_FREE_NULL if free cannot free NULL.
+   * modules/free (configure.ac): Also replace 'free' if
+   it does not preserve errno.
+
idx: simplify IDX_MAX, remove IDX_WIDTH
* lib/idx.h (IDX_MAX): Simplify by removing obsolete reference
to UNSIGNED_IDX_T.
diff --git a/lib/free.c b/lib/free.c
index 50a300ffa..843ce4816 100644
--- a/lib/free.c
+++ b/lib/free.c
@@ -22,9 +22,17 @@
 
 #include 
 
+#include 
+
 void
 rpl_free (void *p)
 {
-  if (p)
-free (p);
+#ifdef CANNOT_FREE_NULL
+  if (!p)
+return;
+#endif
+
+  int err = errno;
+  free (p);
+  errno = err;
 }
diff --git a/m4/free.m4 b/m4/free.m4
index a62214cd1..62daa8ecc 100644
--- a/m4/free.m4
+++ b/m4/free.m4
@@ -35,10 +35,43 @@ AC_DEFUN([gl_FUNC_FREE],
  esac
 ])
 
+  dnl In the next release of POSIX, free must preserve errno.
+  dnl https://www.austingroupbugs.net/view.php?id=385
+  dnl https://sourceware.org/bugzilla/show_bug.cgi?id=17924
+  dnl For now, assume implementations other than glibc do not preserve errno
+  dnl unless they set _POSIX_VERSION to the next release number,
+  dnl whatever that happens to be.
+  AC_CACHE_CHECK([whether free is known to preserve errno],
+[gl_cv_func_free_preserves_errno],
+[case $host_os in
+   *-gnu* | gnu*)
+ gl_cv_func_free_preserves_errno=yes;;
+   *)
+ AC_COMPILE_IFELSE(
+   [AC_LANG_PROGRAM(
+  [[#include 
+  ]],
+  [[#if _POSIX_VERSION <= 200809
+  #error "'free' is not known to preserve errno"
+#endif
+  ]])],
+   [gl_cv_func_free_preserves_errno=yes],
+   [gl_cv_func_free_preserves_errno=no]);;
+ esac
+])
+
   if test $gl_cv_func_free = no; then
+AC_DEFINE([CANNOT_FREE_NULL], [1],
+  [Define to 1 if free (NULL) does not work.])
+  fi
+
+  case $gl_cv_func_free,$gl_cv_func_free_preserves_errno in
+   *yes,*yes) ;;
+   *)
 AC_DEFINE([free], [rpl_free],
   [Define to rpl_free if the replacement function should be used.])
-  fi
+;;
+  esac
 ])
 
 # Prerequisites of lib/free.c.
diff --git a/modules/free b/modules/free
index ec91f8889..ea2509821 100644
--- a/modules/free
+++ b/modules/free
@@ -1,8 +1,5 @@
 Description:
-Work around incompatibility on older systems where free (NULL) fails.
-
-Notice:
-This module is obsolete.
+Work around systems where free sets errno or free (NULL) fails.
 
 Files:
 lib/free.c
@@ -12,10 +9,13 @@ Depends-on:
 
 configure.ac:
 gl_FUNC_FREE
-if test $gl_cv_func_free = no; then
+case $gl_cv_func_free,$gl_cv_func_free_errno in
+ *yes,*yes) ;;
+ *)
   AC_LIBOBJ([free])
   gl_PREREQ_FREE
-fi
+  ;;
+esac
 
 Makefile.am:
 
-- 
2.27.0