Patchwork [RFC,v2,1/3] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg

login
register
mail settings
Submitter Gabriel F. T. Gomes
Date June 5, 2018, 10:21 p.m.
Message ID <20180605222120.24696-2-gabriel@inconstante.eti.br>
Download mbox | patch
Permalink /patch/27653/
State New
Headers show

Comments

Gabriel F. T. Gomes - June 5, 2018, 10:21 p.m.
On powerpc64le, long double can currently take two formats: the same as
double (-mlong-double-64) or IBM Extended Precision (default with
-mlong-double-128 or explicitly with -mabi=ibmlongdouble).  The internal
implementation of printf-like functions is aware of these possibilites
and properly parses floating-point values from the variable arguments,
before making calls to __printf_fp and __printf_fphex.  These functions
are also aware of the format possibilites and know how to convert both
formats to string.

When library support for TS 18661-3 was added to glibc, __printf_fp and
__printf_fphex were extented with support for an additional type
(__float128/_Float128) with a different format (binary128).  Now that
powerpc64le is getting support for its third long double format, and
taking into account that this format is the same as the format of
__float128/_Float128, this patch extends __vfprintf_internal to properly
call __printf_fp and __printf_fphex with this new format.

Tested for powerpc64le (with additional patches to actually enable the
use of these preparations) and for x86_64.

	* libio/libioP.h (PRINTF_LDBL_USES_FLOAT128): New macro to be
	used as a mask for the mode argument of __vfprintf_internal.
	* stdio-common/printf-parse.h (printf_arg): New union member:
	pa_float128.
	* stdio-common/vfprintf-internal.c
	(LDBL_USES_FLOAT128): New macro.
	(PARSE_FLOAT_VA_ARG_EXTENDED): Likewise.
	(PARSE_FLOAT_VA_ARG): Likewise.
	(SETUP_FLOAT128_INFO): Likewise.
	(process_arg): Use PARSE_FLOAT_VA_ARG_EXTENDED and
	SETUP_FLOAT128_INFO.
	[__HAVE_FLOAT128_UNLIKE_LDBL] (printf_positional): Write
	floating-point value to the new union member, pa_float128.
---
 libio/libioP.h                   |  5 ++--
 stdio-common/printf-parse.h      |  3 ++
 stdio-common/vfprintf-internal.c | 62 +++++++++++++++++++++++++++++++---------
 3 files changed, 55 insertions(+), 15 deletions(-)
Joseph Myers - June 6, 2018, 3:07 p.m.
On Tue, 5 Jun 2018, Gabriel F. T. Gomes wrote:

> +#define LDBL_USES_FLOAT128 ((mode_flags && PRINTF_LDBL_USES_FLOAT128) == 1)

That && needs to be &, at which point you need to test != 0 rather than == 
1.

> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +# define SETUP_FLOAT128_INFO(INFO)					      \
> +  if (LDBL_USES_FLOAT128)						      \
> +    INFO.is_binary128 = 1;						      \
> +  else									      \
> +    INFO.is_binary128 = 0;
> +#else
> +# define SETUP_FLOAT128_INFO(INFO)					      \
> +  INFO.is_binary128 = 0;
> +#endif

I think the first case here is wrong.  As I understand it, this macro is 
used in the case of positional parameters or registered printf handlers, 
where the string is first parsed (via printf-parsemb.c, which has no 
special handling of different long double formats), and then the arguments 
are extracted in order, and then the format specifiers are processed with 
the arguments that were extracted.  In particular, the code using it

> @@ -789,8 +821,7 @@ static const uint8_t jump_table[] =
>  		fspec->data_arg_type = PA_DOUBLE;			      \
>  		fspec->info.is_long_double = 0;				      \
>  	      }								      \
> -	    /* Not supported by *printf functions.  */			      \
> -	    fspec->info.is_binary128 = 0;				      \
> +	    SETUP_FLOAT128_INFO (fspec->info)				      \
>  									      \
>  	    function_done = __printf_fp (s, &fspec->info, &ptr);	      \
>  	  }								      \

(and likewise for hex floats) executes for a double argument, as well as 
long double.  So what I think you actually need to do is copy the 
is_long_double setting to is_binary128 in the LDBL_USES_FLOAT128 case, 
rather than setting it to 1 unconditionally.

This illustrates the need for more thorough tests of vfprintf, which 
should test double arguments as well as long double, and test the use of 
positional arguments.  (Given such thorough tests for one printf-family 
function, other printf-family functions could then have less detailed 
tests that suffice to verify the correct long double format gets handled.)

> +#if __HAVE_FLOAT128_UNLIKE_LDBL
> +	else if (LDBL_USES_FLOAT128)
> +	  args_value[cnt].pa_float128 = va_arg (*ap_savep, __float128);
> +#endif

I think you should generally be using _Float128, not __float128, in such 
code.

Patch

diff --git a/libio/libioP.h b/libio/libioP.h
index 07b60e3e26..a0353f76fe 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -687,8 +687,9 @@  extern int __vswprintf_internal (wchar_t *string, size_t maxlen,
                                  unsigned int mode_flags);
 
 /* Flags for __v*printf_internal.  */
-#define PRINTF_LDBL_IS_DBL 0x0001
-#define PRINTF_FORTIFY     0x0002
+#define PRINTF_LDBL_IS_DBL		0x0001
+#define PRINTF_FORTIFY			0x0002
+#define PRINTF_LDBL_USES_FLOAT128	0x0004
 
 extern size_t _IO_getline (FILE *,char *, size_t, int, int);
 libc_hidden_proto (_IO_getline)
diff --git a/stdio-common/printf-parse.h b/stdio-common/printf-parse.h
index e07186ec83..7f2fd56049 100644
--- a/stdio-common/printf-parse.h
+++ b/stdio-common/printf-parse.h
@@ -57,6 +57,9 @@  union printf_arg
     unsigned long long int pa_u_long_long_int;
     double pa_double;
     long double pa_long_double;
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+    __float128 pa_float128;
+#endif
     const char *pa_string;
     const wchar_t *pa_wstring;
     void *pa_pointer;
diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index 2d9016e83b..fb784f538c 100644
--- a/stdio-common/vfprintf-internal.c
+++ b/stdio-common/vfprintf-internal.c
@@ -68,8 +68,43 @@ 
     } while (0)
 #define UNBUFFERED_P(S) ((S)->_flags & _IO_UNBUFFERED)
 #define LDBL_IS_DBL (__glibc_unlikely ((mode_flags & PRINTF_LDBL_IS_DBL) != 0))
+#define LDBL_USES_FLOAT128 ((mode_flags && PRINTF_LDBL_USES_FLOAT128) == 1)
 #define DO_FORTIFY  ((mode_flags & PRINTF_FORTIFY) != 0)
 
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO)				      \
+  if (LDBL_USES_FLOAT128)						      \
+    {									      \
+      INFO.is_binary128 = 1;						      \
+      the_arg.pa_float128 = va_arg (ap, __float128);			      \
+    }									      \
+  else									      \
+    {									      \
+      PARSE_FLOAT_VA_ARG (INFO)						      \
+    }
+#else
+# define PARSE_FLOAT_VA_ARG_EXTENDED(INFO)				      \
+  PARSE_FLOAT_VA_ARG (INFO)
+#endif
+
+#define PARSE_FLOAT_VA_ARG(INFO)					      \
+  INFO.is_binary128 = 0;						      \
+  if (is_long_double)							      \
+    the_arg.pa_long_double = va_arg (ap, long double);			      \
+  else									      \
+    the_arg.pa_double = va_arg (ap, double);
+
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+# define SETUP_FLOAT128_INFO(INFO)					      \
+  if (LDBL_USES_FLOAT128)						      \
+    INFO.is_binary128 = 1;						      \
+  else									      \
+    INFO.is_binary128 = 0;
+#else
+# define SETUP_FLOAT128_INFO(INFO)					      \
+  INFO.is_binary128 = 0;
+#endif
+
 #define done_add(val) \
   do {									      \
     unsigned int _val = val;						      \
@@ -773,10 +808,7 @@  static const uint8_t jump_table[] =
 					.wide = sizeof (CHAR_T) != 1,	      \
 					.is_binary128 = 0};		      \
 									      \
-	    if (is_long_double)						      \
-	      the_arg.pa_long_double = va_arg (ap, long double);	      \
-	    else							      \
-	      the_arg.pa_double = va_arg (ap, double);			      \
+	    PARSE_FLOAT_VA_ARG_EXTENDED (info)				      \
 	    ptr = (const void *) &the_arg;				      \
 									      \
 	    function_done = __printf_fp (s, &info, &ptr);		      \
@@ -789,8 +821,7 @@  static const uint8_t jump_table[] =
 		fspec->data_arg_type = PA_DOUBLE;			      \
 		fspec->info.is_long_double = 0;				      \
 	      }								      \
-	    /* Not supported by *printf functions.  */			      \
-	    fspec->info.is_binary128 = 0;				      \
+	    SETUP_FLOAT128_INFO (fspec->info)				      \
 									      \
 	    function_done = __printf_fp (s, &fspec->info, &ptr);	      \
 	  }								      \
@@ -833,10 +864,7 @@  static const uint8_t jump_table[] =
 					.wide = sizeof (CHAR_T) != 1,	      \
 					.is_binary128 = 0};		      \
 									      \
-	    if (is_long_double)						      \
-	      the_arg.pa_long_double = va_arg (ap, long double);	      \
-	    else							      \
-	      the_arg.pa_double = va_arg (ap, double);			      \
+	    PARSE_FLOAT_VA_ARG_EXTENDED (info)				      \
 	    ptr = (const void *) &the_arg;				      \
 									      \
 	    function_done = __printf_fphex (s, &info, &ptr);		      \
@@ -846,8 +874,7 @@  static const uint8_t jump_table[] =
 	    ptr = (const void *) &args_value[fspec->data_arg];		      \
 	    if (LDBL_IS_DBL)                                            \
 	      fspec->info.is_long_double = 0;				      \
-	    /* Not supported by *printf functions.  */			      \
-	    fspec->info.is_binary128 = 0;				      \
+	    SETUP_FLOAT128_INFO (fspec->info)				      \
 									      \
 	    function_done = __printf_fphex (s, &fspec->info, &ptr);	      \
 	  }								      \
@@ -1871,6 +1898,10 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
 	    args_value[cnt].pa_double = va_arg (*ap_savep, double);
 	    args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
 	  }
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+	else if (LDBL_USES_FLOAT128)
+	  args_value[cnt].pa_float128 = va_arg (*ap_savep, __float128);
+#endif
 	else
 	  args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
 	break;
@@ -1889,7 +1920,12 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
 	      (args_value[cnt].pa_user, ap_savep);
 	  }
 	else
-	  args_value[cnt].pa_long_double = 0.0;
+	  {
+	    args_value[cnt].pa_long_double = 0.0;
+#if __HAVE_FLOAT128_UNLIKE_LDBL
+	    args_value[cnt].pa_float128 = 0;
+#endif
+	  }
 	break;
       case -1:
 	/* Error case.  Not all parameters appear in N$ format