Patchwork [26/31] Do not redirect calls to __GI_* symbols, when redirecting to *ieee128

login
register
mail settings
Submitter Gabriel F. T. Gomes
Date Oct. 15, 2019, 7:05 p.m.
Message ID <20191015190529.11559-27-gabriel@inconstante.net.br>
Download mbox | patch
Permalink /patch/35015/
State New
Headers show

Comments

Gabriel F. T. Gomes - Oct. 15, 2019, 7:05 p.m.
From: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>

On platforms where long double has IEEE binary128 format as a third
option (initially, only powerpc64le), many exported functions are
redirected to their __*ieee128 equivalents.  This redirection is
provided by installed headers such as stdio-ldbl.h, and is supposed to
work correctly with user code.

However, during the build of glibc, similar redirections are employed,
in internal headers such as include/stdio.h, in order to avoid extra PLT
entries.  These redirections conflict with the redirections to
__*ieee128, and must be avoided during the build.  This patch protects
the second redirections with a test for __LONG_DOUBLE_USES_FLOAT128.
---
 include/err.h        |  3 +++
 include/stdio.h      | 14 ++++++++++++--
 include/sys/syslog.h |  5 +++++
 include/wchar.h      |  3 +++
 4 files changed, 23 insertions(+), 2 deletions(-)
Florian Weimer - Oct. 16, 2019, 3:57 a.m.
* Gabriel F. T. Gomes:

> From: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
>
> On platforms where long double has IEEE binary128 format as a third
> option (initially, only powerpc64le), many exported functions are
> redirected to their __*ieee128 equivalents.  This redirection is
> provided by installed headers such as stdio-ldbl.h, and is supposed to
> work correctly with user code.
>
> However, during the build of glibc, similar redirections are employed,
> in internal headers such as include/stdio.h, in order to avoid extra PLT
> entries.  These redirections conflict with the redirections to
> __*ieee128, and must be avoided during the build.  This patch protects
> the second redirections with a test for __LONG_DOUBLE_USES_FLOAT128.

Why don't we see this problem with the existing dual ABIs for long
double?

> diff --git a/include/stdio.h b/include/stdio.h
> index bea2066cd1..8ec801c989 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -13,7 +13,10 @@ extern int __fcloseall (void) attribute_hidden;
>  extern int __snprintf (char *__restrict __s, size_t __maxlen,
>  		       const char *__restrict __format, ...)
>       __attribute__ ((__format__ (__printf__, 3, 4)));
> +#  if !defined __LONG_DOUBLE_USES_FLOAT128 \
> +  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
>  libc_hidden_proto (__snprintf)
> +#  endif
>  extern int __vfscanf (FILE *__restrict __s,
>  		      const char *__restrict __format,
>  		      __gnuc_va_list __arg)

This leads to a build failure on most architectures because
__LONG_DOUBLE_USES_FLOAT128 is not defined at this point.

(I tried the gabriel/powerpc-ieee128-printscan branch at commit
1c4f7fffc4f1cc186906dd47812e725e51bb036a.)

Thanks,
Florian
Gabriel F. T. Gomes - Oct. 21, 2019, 1:48 a.m.
Hi, Florian,

On Wed, 16 Oct 2019, Florian Weimer wrote:

>Why don't we see this problem with the existing dual ABIs for long
>double?

That would be because, on the previous transition (from long double being
the same as double to long double with some other, whatever format), the
old format was the same as double, which meant that no files had to be
compiled in the old mode (-mlong-double-64).  The [at-the-time-]new compat
symbols got created during the compilation of the double-typed functions,
which could be built with -mlong-double-128.

For the new ABI, we rely on a lot of builds with -mlong-double-128 and
-mfloat128, which work fine without this patch.  However, we need a couple
of builds in -mabi=ieeelongdouble mode, and the use of this flag causes
build errors such as these:

  In file included from ../sysdeps/ieee754/ldbl-128ibm-compat/ieee128-qefgcvt.c:20:
  ../include/stdio.h:174:1: error: asm declaration ignored due to conflict with previous rename [-Werror=pragmas]
    174 | libc_hidden_proto (dprintf)
        | ^~~~~~~~~~~~~~~~~
  ../include/stdio.h:178:1: error: asm declaration ignored due to conflict with previous rename [-Werror=pragmas]
    178 | libc_hidden_proto (fprintf)
        | ^~~~~~~~~~~~~~~~~
  ../include/stdio.h:179:1: error: asm declaration ignored due to conflict with previous rename [-Werror=pragmas]
    179 | libc_hidden_proto (vfprintf)
        | ^~~~~~~~~~~~~~~~~
  ../include/stdio.h:180:1: error: asm declaration ignored due to conflict with previous rename [-Werror=pragmas]
    180 | libc_hidden_proto (sprintf)
        | ^~~~~~~~~~~~~~~~~
  cc1: all warnings being treated as errors
  make[2]: *** [/home/gabriel/build/powerpc64le/glibc/sysd-rules:1663: /home/gabriel/build/powerpc64le/glibc/misc/ieee128-qefgcvt.os] Error 1
  make[2]: Leaving directory '/home/gabriel/src/glibc/misc'

When we started working on this effort, we went on a path that led to a
lot more files being built with -mabi=ieeelongdouble, now it's just cvt*
functions, really.  So I'm updating this patch to touch less files.

>> +#  if !defined __LONG_DOUBLE_USES_FLOAT128 \
>> +  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
>>  libc_hidden_proto (__snprintf)
>> +#  endif
>>  extern int __vfscanf (FILE *__restrict __s,
>>  		      const char *__restrict __format,
>>  		      __gnuc_va_list __arg)  
>
>This leads to a build failure on most architectures because
>__LONG_DOUBLE_USES_FLOAT128 is not defined at this point.
>
>(I tried the gabriel/powerpc-ieee128-printscan branch at commit
>1c4f7fffc4f1cc186906dd47812e725e51bb036a.)

I'm sorry.  It is perfectly reasonable that I should have tested it on
other platforms before sending this patchset.  I'm fixing this and testing
on more platforms using build-many-glibcs.py (using b-m-g sent me on a
search for *other* problems with the patchset, so thanks for testing the
branch, much appreciated :).

The actual snippet that causes the problem is the following:

>@@ -72,7 +75,8 @@ libc_hidden_proto (__isoc99_vfscanf)
>    Unfortunately, symbol redirection is not transitive, so the
>    __REDIRECT in the public header does not link up with the above
>    libc_hidden_proto.  Bridge the gap with a macro.  */
>-#  if !__GLIBC_USE (DEPRECATED_SCANF)
>+#  if !__GLIBC_USE (DEPRECATED_SCANF) \
>+      && __LONG_DOUBLE_USES_FLOAT128 == 0
> #   undef sscanf
> #   define sscanf __isoc99_sscanf
> #  endif

It's missing the check for __LONG_DOUBLE_USES_FLOAT128 being defined.  I
have locally changed this to a check for the macro being defined, then for
a value (which solves the problem according to b-m-g), but I'm wondering if
I should switch to defining it to 0 (zero) on all other long-double.h
files (for all platforms), thus avoiding the problems with undefined
macros [1].  Do you have an opinion on that?

I'll send a v2, soon.

[1] https://sourceware.org/glibc/wiki/Wundef
Florian Weimer - Oct. 21, 2019, 10:12 a.m.
* Gabriel F. T. Gomes:

> It's missing the check for __LONG_DOUBLE_USES_FLOAT128 being defined.  I
> have locally changed this to a check for the macro being defined, then for
> a value (which solves the problem according to b-m-g), but I'm wondering if
> I should switch to defining it to 0 (zero) on all other long-double.h
> files (for all platforms), thus avoiding the problems with undefined
> macros [1].  Do you have an opinion on that?

Defining it everywhere is currently what we prefer.  We have many
exceptions (SHARED, the __ASSUME_* macros), but they are supposed to bhe
historic.

Thanks,
Florian

Patch

diff --git a/include/err.h b/include/err.h
index 7c05cd1dbb..4bfd0f1769 100644
--- a/include/err.h
+++ b/include/err.h
@@ -12,12 +12,15 @@  __vwarn_internal (const char *format, __gnuc_va_list ap,
 
 # ifndef _ISOMAC
 
+#if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
 libc_hidden_proto (warn)
 libc_hidden_proto (warnx)
 libc_hidden_proto (vwarn)
 libc_hidden_proto (vwarnx)
 libc_hidden_proto (verr)
 libc_hidden_proto (verrx)
+#  endif
 
 # endif /* !_ISOMAC */
 #endif /* err.h */
diff --git a/include/stdio.h b/include/stdio.h
index bea2066cd1..8ec801c989 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -13,7 +13,10 @@  extern int __fcloseall (void) attribute_hidden;
 extern int __snprintf (char *__restrict __s, size_t __maxlen,
 		       const char *__restrict __format, ...)
      __attribute__ ((__format__ (__printf__, 3, 4)));
+#  if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
 libc_hidden_proto (__snprintf)
+#  endif
 extern int __vfscanf (FILE *__restrict __s,
 		      const char *__restrict __format,
 		      __gnuc_va_list __arg)
@@ -72,7 +75,8 @@  libc_hidden_proto (__isoc99_vfscanf)
    Unfortunately, symbol redirection is not transitive, so the
    __REDIRECT in the public header does not link up with the above
    libc_hidden_proto.  Bridge the gap with a macro.  */
-#  if !__GLIBC_USE (DEPRECATED_SCANF)
+#  if !__GLIBC_USE (DEPRECATED_SCANF) \
+      && __LONG_DOUBLE_USES_FLOAT128 == 0
 #   undef sscanf
 #   define sscanf __isoc99_sscanf
 #  endif
@@ -150,7 +154,10 @@  libc_hidden_proto (__libc_readline_unlocked);
 extern const char *const _sys_errlist_internal[] attribute_hidden;
 extern int _sys_nerr_internal attribute_hidden;
 
+#if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
 libc_hidden_proto (__asprintf)
+#endif
 #  if IS_IN (libc)
 extern FILE *_IO_new_fopen (const char*, const char*);
 #   define fopen(fname, mode) _IO_new_fopen (fname, mode)
@@ -171,13 +178,16 @@  extern int _IO_new_fgetpos (FILE *, __fpos_t *);
 #   define fgetpos(fp, posp) _IO_new_fgetpos (fp, posp)
 #  endif
 
-libc_hidden_proto (dprintf)
 extern __typeof (dprintf) __dprintf
      __attribute__ ((__format__ (__printf__, 2, 3)));
 libc_hidden_proto (__dprintf)
+#if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
+libc_hidden_proto (dprintf)
 libc_hidden_proto (fprintf)
 libc_hidden_proto (vfprintf)
 libc_hidden_proto (sprintf)
+#endif
 libc_hidden_proto (fwrite)
 libc_hidden_proto (perror)
 libc_hidden_proto (remove)
diff --git a/include/sys/syslog.h b/include/sys/syslog.h
index 89d3479ebc..e10c58f6ef 100644
--- a/include/sys/syslog.h
+++ b/include/sys/syslog.h
@@ -3,7 +3,12 @@ 
 #include <misc/sys/syslog.h>
 #ifndef _ISOMAC
 
+#include <bits/floatn.h>
+
+#if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
 libc_hidden_proto (syslog)
+#endif
 
 /* __vsyslog_internal uses the same mode_flags bits as
    __v*printf_internal; see libio/libioP.h.  */
diff --git a/include/wchar.h b/include/wchar.h
index 2cb44954fc..4875c9e84c 100644
--- a/include/wchar.h
+++ b/include/wchar.h
@@ -114,7 +114,10 @@  libc_hidden_proto (fputws_unlocked)
 libc_hidden_proto (putwc_unlocked)
 libc_hidden_proto (putwc)
 
+#if !defined __LONG_DOUBLE_USES_FLOAT128 \
+  || (defined __LONG_DOUBLE_USES_FLOAT128 && __LONG_DOUBLE_USES_FLOAT128 == 0)
 libc_hidden_proto (vswscanf)
+#endif
 
 libc_hidden_proto (mbrtowc)
 libc_hidden_proto (wcrtomb)