[PATCH v2] handle libc implemntations which do not provide `error.h`

[PATCH v2] handle libc implemntations which do not provide `error.h`

[Saleem Abdulrasool]

Introduce a configure time check for the presence of `error.h`.  In the
case that `error.h` is not available, we can fall back to `err.h`.
Although `err.h` is not a C standard header (it is a BSD extension),
many libc implementations provide.  If there are targets which do not
provide an implementation of `err.h`, it would be possible to further
extend the implementation to be more portable.

This resolves bug #21008.

Signed-off-by: Saleem Abdulrasool <abdulras@google.com>
---
 configure.ac |  3 +++
 lib/system.h | 26 +++++++++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 7caff2c5..177bb1a2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -431,6 +431,9 @@ AC_CHECK_DECLS([reallocarray],[],[],
 
 AC_CHECK_FUNCS([process_vm_readv])
 
+AC_CHECK_HEADERS([error.h])
+AC_CHECK_HEADERS([err.h])
+
 old_CFLAGS="$CFLAGS"
 CFLAGS="$CFLAGS -D_GNU_SOURCE"
 AC_FUNC_STRERROR_R()
diff --git a/lib/system.h b/lib/system.h
index 58d9deee..b963fd15 100644
--- a/lib/system.h
+++ b/lib/system.h
@@ -29,8 +29,9 @@
 #ifndef LIB_SYSTEM_H
 #define LIB_SYSTEM_H	1
 
+#include <config.h>
+
 #include <errno.h>
-#include <error.h>
 #include <stddef.h>
 #include <stdint.h>
 #include <sys/param.h>
@@ -38,8 +39,31 @@
 #include <byteswap.h>
 #include <unistd.h>
 #include <string.h>
+#include <stdarg.h>
 #include <stdlib.h>
 
+#if defined(HAVE_ERROR_H)
+#include <error.h>
+#elif defined(HAVE_ERR_H)
+#include <err.h>
+
+static int error_message_count = 0;
+
+static inline void error(int status, int errnum, const char *format, ...) {
+  va_list argp;
+
+  va_start(argp, format);
+  verr(status, format, argp);
+  va_end(argp);
+
+  if (status)
+    exit(status);
+  ++error_message_count;
+}
+#else
+#error "err.h or error.h must be available"
+#endif
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 # define LE32(n)	(n)
 # define LE64(n)	(n)
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v2] handle libc implemntations which do not provide `error.h`

[Saleem Abdulrasool]

I think that this is not exactly ideal, as it will introduce a local
error_message_count in each translation unit, rather than giving it
vague linkage as I had hoped.  I think it may be better to introduce a new
source file here.  I can move the implementation around though.

A second issue is that playing with this further, it doesn't fully resolve
the PR as this only fixes it for libelf (which I realized only recently).

On Fri, Aug 20, 2021 at 1:28 PM Saleem Abdulrasool <abdulras@google.com>
wrote:

> Introduce a configure time check for the presence of `error.h`.  In the
> case that `error.h` is not available, we can fall back to `err.h`.
> Although `err.h` is not a C standard header (it is a BSD extension),
> many libc implementations provide.  If there are targets which do not
> provide an implementation of `err.h`, it would be possible to further
> extend the implementation to be more portable.
>
> This resolves bug #21008.
>
> Signed-off-by: Saleem Abdulrasool <abdulras@google.com>
> ---
>  configure.ac |  3 +++
>  lib/system.h | 26 +++++++++++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7caff2c5..177bb1a2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -431,6 +431,9 @@ AC_CHECK_DECLS([reallocarray],[],[],
>
>  AC_CHECK_FUNCS([process_vm_readv])
>
> +AC_CHECK_HEADERS([error.h])
> +AC_CHECK_HEADERS([err.h])
> +
>  old_CFLAGS="$CFLAGS"
>  CFLAGS="$CFLAGS -D_GNU_SOURCE"
>  AC_FUNC_STRERROR_R()
> diff --git a/lib/system.h b/lib/system.h
> index 58d9deee..b963fd15 100644
> --- a/lib/system.h
> +++ b/lib/system.h
> @@ -29,8 +29,9 @@
>  #ifndef LIB_SYSTEM_H
>  #define LIB_SYSTEM_H   1
>
> +#include <config.h>
> +
>  #include <errno.h>
> -#include <error.h>
>  #include <stddef.h>
>  #include <stdint.h>
>  #include <sys/param.h>
> @@ -38,8 +39,31 @@
>  #include <byteswap.h>
>  #include <unistd.h>
>  #include <string.h>
> +#include <stdarg.h>
>  #include <stdlib.h>
>
> +#if defined(HAVE_ERROR_H)
> +#include <error.h>
> +#elif defined(HAVE_ERR_H)
> +#include <err.h>
> +
> +static int error_message_count = 0;
> +
> +static inline void error(int status, int errnum, const char *format, ...)
> {
> +  va_list argp;
> +
> +  va_start(argp, format);
> +  verr(status, format, argp);
> +  va_end(argp);
> +
> +  if (status)
> +    exit(status);
> +  ++error_message_count;
> +}
> +#else
> +#error "err.h or error.h must be available"
> +#endif
> +
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>  # define LE32(n)       (n)
>  # define LE64(n)       (n)
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
>

Re: [PATCH v2] handle libc implemntations which do not provide `error.h`

[Mark Wielaard]

Hi Saleem,

On Fri, 2021-08-27 at 08:24 -0700, Saleem Abdulrasool via Elfutils-devel wrote:
> I think that this is not exactly ideal, as it will introduce a local
> error_message_count in each translation unit, rather than giving it
> vague linkage as I had hoped.  I think it may be better to introduce a new
> source file here.  I can move the implementation around though.
> 
> A second issue is that playing with this further, it doesn't fully resolve
> the PR as this only fixes it for libelf (which I realized only recently).

Oops. Our messages crossed and I just pushed:

commit 76c84c137a82a7cacbc69b1696052491b3bb81cb
Author: Saleem Abdulrasool <abdulras@google.com>
Date:   Fri Aug 20 20:28:23 2021 +0000

    handle libc implementations which do not provide `error.h`
    
    Introduce a configure time check for the presence of `error.h`.  In the
    case that `error.h` is not available, we can fall back to `err.h`.
    Although `err.h` is not a C standard header (it is a BSD extension),
    many libc implementations provide.  If there are targets which do not
    provide an implementation of `err.h`, it would be possible to further
    extend the implementation to be more portable.
    
    This resolves bug #21008.
    
    Signed-off-by: Saleem Abdulrasool <abdulras@google.com>

It passes all local tests and it is currently going through the buildbot:
https://builder.wildebeest.org/buildbot/#/changes/2530
But of course all those systems use normal glibc.

Should I revert the commit?

Thanks,

Mark

Re: [PATCH v2] handle libc implemntations which do not provide `error.h`

[Saleem Abdulrasool]

We have two choices:
- revert and use the updated patch I already sent
- I can do a follow up change to migrate the code

I don't particularly have a strong opinion on which approach we take, since
the commit has already been done.  Since glibc systems are completely
untouched by the changes neither path should cause problems for bisection
in the future for previously supported systems.  I'd go with whatever is
most commonly done in this project.

On Fri, Aug 27, 2021 at 8:39 AM Mark Wielaard <mark@klomp.org> wrote:

> Hi Saleem,
>
> On Fri, 2021-08-27 at 08:24 -0700, Saleem Abdulrasool via Elfutils-devel
> wrote:
> > I think that this is not exactly ideal, as it will introduce a local
> > error_message_count in each translation unit, rather than giving it
> > vague linkage as I had hoped.  I think it may be better to introduce a
> new
> > source file here.  I can move the implementation around though.
> >
> > A second issue is that playing with this further, it doesn't fully
> resolve
> > the PR as this only fixes it for libelf (which I realized only recently).
>
> Oops. Our messages crossed and I just pushed:
>
> commit 76c84c137a82a7cacbc69b1696052491b3bb81cb
> Author: Saleem Abdulrasool <abdulras@google.com>
> Date:   Fri Aug 20 20:28:23 2021 +0000
>
>     handle libc implementations which do not provide `error.h`
>
>     Introduce a configure time check for the presence of `error.h`.  In the
>     case that `error.h` is not available, we can fall back to `err.h`.
>     Although `err.h` is not a C standard header (it is a BSD extension),
>     many libc implementations provide.  If there are targets which do not
>     provide an implementation of `err.h`, it would be possible to further
>     extend the implementation to be more portable.
>
>     This resolves bug #21008.
>
>     Signed-off-by: Saleem Abdulrasool <abdulras@google.com>
>
> It passes all local tests and it is currently going through the buildbot:
> https://builder.wildebeest.org/buildbot/#/changes/2530
> But of course all those systems use normal glibc.
>
> Should I revert the commit?
>
> Thanks,
>
> Mark
>