Re: [PATCH] Added Redirects to longdouble error functions [BZ #29033]

[Paul E Murphy <murphyp@linux.ibm.com>]

On 3/4/23 11:45 AM, Sachin Monga via Libc-alpha wrote:
> This patch redirects the error functions to the appropriate
> longdouble variants which enables the compiler to optimize
> for the abi ieeelongdouble.
> 
> Signed-off-by: Sachin Monga <smonga@linux.ibm.com>
> ---
>   misc/Makefile                               |   2 +-
>   misc/bits/error-ldbl.h                      |  53 ++++++++-
>   misc/sys/cdefs.h                            |   3 +-
>   misc/tst-ldbl-errorfptr.c                   | 114 ++++++++++++++++++++
>   sysdeps/ieee754/ldbl-128ibm-compat/Makefile |   3 +
>   sysdeps/ieee754/ldbl-opt/Makefile           |   5 +
>   6 files changed, 176 insertions(+), 4 deletions(-)
>   create mode 100644 misc/tst-ldbl-errorfptr.c
> 
> diff --git a/misc/Makefile b/misc/Makefile
> index 1a09f777fa..9f42321206 100644
> --- a/misc/Makefile
> +++ b/misc/Makefile
> @@ -90,7 +90,7 @@ tests := tst-dirname tst-tsearch tst-fdset tst-mntent tst-hsearch \
>   	 tst-preadvwritev2 tst-preadvwritev64v2 tst-warn-wide \
>   	 tst-ldbl-warn tst-ldbl-error tst-dbl-efgcvt tst-ldbl-efgcvt \
>   	 tst-mntent-autofs tst-syscalls tst-mntent-escape tst-select \
> -	 tst-ioctl
> +	 tst-ioctl tst-ldbl-errorfptr
> 
>   tests-time64 := \
>     tst-select-time64 \
> diff --git a/misc/bits/error-ldbl.h b/misc/bits/error-ldbl.h
> index 599a7d6e06..638e030c96 100644
> --- a/misc/bits/error-ldbl.h
> +++ b/misc/bits/error-ldbl.h
> @@ -20,5 +20,54 @@
>   # error "Never include <bits/error-ldbl.h> directly; use <error.h> instead."
>   #endif
> 
> -__LDBL_REDIR_DECL (error)
> -__LDBL_REDIR_DECL (error_at_line)
> +
> +extern void __REDIRECT_LDBL (__error_alias, (int __status, int __errnum,
> +					const char *__format, ...),
> +			error)
> +  __attribute__ ((__format__ (__printf__, 3, 4)));
> +extern void __REDIRECT_LDBL (__error_noreturn, (int __status, int __errnum,
> +					   const char *__format, ...),
> +			error)
> +  __attribute__ ((__noreturn__, __format__ (__printf__, 3, 4)));
> +
> +
> +/* If we know the function will never return make sure the compiler
> +   realizes that, too.  */
> +__extern_always_inline void
> +error (int __status, int __errnum, const char *__format, ...)
> +{
> +  if (__builtin_constant_p (__status) && __status != 0)
> +    __error_noreturn (__status, __errnum, __format, __va_arg_pack ());
> +  else
> +    __error_alias (__status, __errnum, __format, __va_arg_pack ());
> +}
> +
> +
> +extern void __REDIRECT_LDBL (__error_at_line_alias, (int __status, int __errnum,
> +						const char *__fname,
> +						unsigned int __line,
> +						const char *__format, ...),
> +			error_at_line)
> +  __attribute__ ((__format__ (__printf__, 5, 6)));
> +extern void __REDIRECT_LDBL (__error_at_line_noreturn, (int __status, int __errnum,
> +						   const char *__fname,
> +						   unsigned int __line,
> +						   const char *__format,
> +						   ...),
> +			error_at_line)
> +  __attribute__ ((__noreturn__, __format__ (__printf__, 5, 6)));
> +
> +
> +/* If we know the function will never return make sure the compiler
> +   realizes that, too.  */
> +__extern_always_inline void
> +error_at_line (int __status, int __errnum, const char *__fname,
> +	       unsigned int __line, const char *__format, ...)
> +{
> +  if (__builtin_constant_p (__status) && __status != 0)
> +    __error_at_line_noreturn (__status, __errnum, __fname, __line, __format,
> +			      __va_arg_pack ());
> +  else
> +    __error_at_line_alias (__status, __errnum, __fname, __line,
> +			   __format, __va_arg_pack ());
> +}
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index 23ec0ebd2a..285191482a 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -569,6 +569,8 @@
>   #  define __LDBL_REDIR(name, proto) ... unused__ldbl_redir
>   #  define __LDBL_REDIR_DECL(name) \
>     extern __typeof (name) name __asm (__ASMNAME ("__" #name "ieee128"));
> +#  define __REDIRECT_LDBL(name, proto, alias) \
> +  name proto __asm (__ASMNAME ("__" #alias "ieee128"))
> 
>   /* Alias name defined automatically, with leading underscores.  */
>   #  define __LDBL_REDIR2_DECL(name) \
> @@ -586,7 +588,6 @@
>     __LDBL_REDIR1_NTH (name, proto, __##alias##ieee128)
> 
>   /* Unused.  */
> -#  define __REDIRECT_LDBL(name, proto, alias) ... unused__redirect_ldbl
>   #  define __LDBL_REDIR_NTH(name, proto) ... unused__ldbl_redir_nth
> 
>   # else
> diff --git a/misc/tst-ldbl-errorfptr.c b/misc/tst-ldbl-errorfptr.c
> new file mode 100644
> index 0000000000..4896694fb9
> --- /dev/null
> +++ b/misc/tst-ldbl-errorfptr.c
> @@ -0,0 +1,114 @@
> +/* Test for the long double conversions in error* functions
> +   when they are returned as function pointer.
> +   Copyright (C) 2018-2023 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <err.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <stdarg.h>
> +#include <string.h>
> +
> +#include <support/capture_subprocess.h>
> +#include <support/check.h>
> +
> +struct tests
> +{
> +  void *callback;
> +  const char *expected;
> +};
> +
> +va_list args;
> +
> +typedef void (*error_func_t) (int ,int ,const char* ,...);
> +typedef void (*error_at_line_func_t) (int ,int ,const char* ,
> +		unsigned int ,const char* ,...);
> +
> +error_func_t
> +get_error_func(void) {
> +  return &error;
> +}
> +
> +error_at_line_func_t
> +get_error_at_line_func(void) {
> +  return &error_at_line;
> +}

You are almost there. Under optimization, this gets inlined and 
transformed into a direct call when I test with AT15.  Maybe a noinline 
attribute is sufficient to prevent this, but I think this test is 
difficult to verify it is testing the desired behavior.

The test could be simplified further.  You really want to verify the 
function pointer is correctly redirected.  E.g for the nldbl case, you 
want to verify:

get_error_at_line_func() == dlsym(RTLD_DEFAULT, "__nldb_error_at_line");
get_error_func() == dlsym(RTLD_DEFAULT, "__nldb_error");

The function and wrappers are already verified by existing tests, so it 
isn't necessary to test the function behind the pointer.


> +
> +static void
> +callback_error (void *closure)
> +{
> +  errno = 0;
> +  error_func_t fp;
> +  fp = get_error_func();
> +  fp (0, 0, "%Lf", (long double) -1);
> +}
> +
> +static void
> +callback_error_at_line (void *closure)
> +{
> +  errno = 0;
> +  error_at_line_func_t fpat;
> +  fpat = get_error_at_line_func();
> +  fpat (0, 0, "", 0, "%Lf", (long double) -1);
> +}
> +
> +static void
> +do_one_test (void *callback, const char *expected, ...)
> +{
> +  struct support_capture_subprocess result;
> +
> +  va_start (args, expected);
> +
> +  /* Call 'callback', which fills in the output and error buffers.  */
> +  result = support_capture_subprocess (callback, NULL);
> +
> +  /* Filter out the name of the program (which should always end with
> +     -errorfptr), so that the test case can be reused by ldbl-opt and
> +     ldbl-128ibm-compat.  */
> +  const char *needle = "-errorfptr:";
> +  char *message;
> +  message = strstr (result.err.buffer, needle);
> +  printf("\nresult.err.buffer=%s",result.err.buffer);
> +  if (message == NULL)
> +    FAIL_EXIT1 ("test case error");
> +  message += strlen (needle);
> +
> +  /* Verify that the output message is as expected.  */
> +  TEST_COMPARE_STRING (message, expected);
> +
> +  va_end (args);
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Test if error and error_at_line are returned as function pointers
> +   * to the callback functions (BZ #29033). */
> +  struct tests tests[] = {
> +    { &callback_error, " -1.000000\n" },
> +    { &callback_error_at_line, ":0: -1.000000\n" }
> +  };
> +
> +  for (int i = 0; i < sizeof (tests) / sizeof (tests[0]); i++)
> +    {
> +      do_one_test (tests[i].callback, tests[i].expected, (long double) -1);
> +    }
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
> index 67d476383a..6aa73d2798 100644
> --- a/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
> +++ b/sysdeps/ieee754/ldbl-128ibm-compat/Makefile
> @@ -252,6 +252,7 @@ CFLAGS-ieee128-qefgcvt_r.c += -mabi=ieeelongdouble -Wno-psabi -mno-gnu-attribute
>   tests-internal += tst-ibm128-warn tst-ieee128-warn
>   tests-internal += tst-ibm128-error tst-ieee128-error
>   tests-internal += tst-ibm128-efgcvt tst-ieee128-efgcvt
> +tests-internal += tst-ibm128-errorfptr tst-ieee128-errorfptr
> 
>   $(objpfx)tst-ibm128-%.c: tst-ldbl-%.c
>   	cp $< $@
> @@ -262,10 +263,12 @@ $(objpfx)tst-ieee128-%.c: tst-ldbl-%.c
>   CFLAGS-tst-ibm128-warn.c += -mabi=ibmlongdouble -Wno-psabi
>   CFLAGS-tst-ibm128-error.c += -mabi=ibmlongdouble -Wno-psabi
>   CFLAGS-tst-ibm128-efgcvt.c += -mabi=ibmlongdouble -Wno-psabi
> +CFLAGS-tst-ibm128-errorfptr.c += -mabi=ibmlongdouble -Wno-psabi
> 
>   CFLAGS-tst-ieee128-warn.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi
>   CFLAGS-tst-ieee128-error.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi
>   CFLAGS-tst-ieee128-efgcvt.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi
> +CFLAGS-tst-ieee128-errorfptr.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi
> 
>   tests-container += test-syslog-ieee128 test-syslog-ibm128
>   CFLAGS-test-syslog-ieee128.c += -mfloat128 -mabi=ieeelongdouble -Wno-psabi
> diff --git a/sysdeps/ieee754/ldbl-opt/Makefile b/sysdeps/ieee754/ldbl-opt/Makefile
> index 1d01846476..1baf065654 100644
> --- a/sysdeps/ieee754/ldbl-opt/Makefile
> +++ b/sysdeps/ieee754/ldbl-opt/Makefile
> @@ -211,6 +211,7 @@ endif
>   ifeq ($(subdir), misc)
>   tests-internal += tst-nldbl-warn
>   tests-internal += tst-nldbl-error
> +tests-internal += tst-nldbl-errorfptr
> 
>   $(objpfx)tst-nldbl-warn.c: tst-ldbl-warn.c
>   	cp $< $@
> @@ -218,6 +219,10 @@ $(objpfx)tst-nldbl-warn.c: tst-ldbl-warn.c
>   $(objpfx)tst-nldbl-error.c: tst-ldbl-error.c
>   	cp $< $@
> 
> +$(objpfx)tst-nldbl-errorfptr.c: tst-ldbl-errorfptr.c
> +	cp $< $@
> +
>   CFLAGS-tst-nldbl-warn.c += -mlong-double-64
>   CFLAGS-tst-nldbl-error.c += -mlong-double-64
> +CFLAGS-tst-nldbl-errorfptr.c += -mlong-double-64
>   endif