Use C2x return value from getpayload of non-NaN (bug 26073)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Use C2x return value from getpayload of non-NaN (bug 26073)

Joseph Myers
In TS 18661-1, getpayload had an unspecified return value for a
non-NaN argument, while C2x requires the return value -1 in that case.

This patch implements the return value of -1.  I don't think this is
worth having a new symbol version that's an alias of the old one,
although occasionally we do that in such cases where the new function
semantics are a refinement of the old ones (to avoid programs relying
on the new semantics running on older glibc versions but not behaving
as intended).

Tested for x86_64 and x86; also ran math/ tests for aarch64 and
powerpc.

---

Carlos, is this OK at the current slush development stage?

diff --git a/manual/arith.texi b/manual/arith.texi
index 89c2c064f1..75eaf67fe7 100644
--- a/manual/arith.texi
+++ b/manual/arith.texi
@@ -1895,9 +1895,10 @@ propagated from NaN inputs to the result of a floating-point
 operation.  These functions, defined by TS 18661-1:2014 and TS
 18661-3:2015, return the payload of the NaN pointed to by @var{x}
 (returned as a positive integer, or positive zero, represented as a
-floating-point number); if @var{x} is not a NaN, they return an
-unspecified value.  They raise no floating-point exceptions even for
-signaling NaNs.
+floating-point number); if @var{x} is not a NaN, they return
+@minus{}1.  They raise no floating-point exceptions even for signaling
+NaNs.  (The return value of @minus{}1 for an argument that is not a
+NaN is specified in C2x; the value was unspecified in TS 18661.)
 @end deftypefun
 
 @deftypefun int setpayload (double *@var{x}, double @var{payload})
diff --git a/math/libm-test-getpayload.inc b/math/libm-test-getpayload.inc
index bd660471aa..72e3c19d1e 100644
--- a/math/libm-test-getpayload.inc
+++ b/math/libm-test-getpayload.inc
@@ -20,17 +20,17 @@
 
 static const struct test_f_f_data getpayload_test_data[] =
   {
-    TEST_fp_f (getpayload, plus_infty, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, minus_infty, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, plus_zero, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, minus_zero, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, 1000, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, max_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, -max_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, min_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, -min_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, min_subnorm_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
-    TEST_fp_f (getpayload, -min_subnorm_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, plus_infty, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, minus_infty, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, plus_zero, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, minus_zero, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, 1000, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, max_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, -max_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, min_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, -min_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, min_subnorm_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
+    TEST_fp_f (getpayload, -min_subnorm_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
 #if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
     TEST_fp_f (getpayload, snan_value_pl ("0x0"), plus_zero, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
     TEST_fp_f (getpayload, -snan_value_pl ("0x0"), plus_zero, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
diff --git a/sysdeps/ieee754/dbl-64/s_getpayload.c b/sysdeps/ieee754/dbl-64/s_getpayload.c
index 3ab89ddd66..5a055be35a 100644
--- a/sysdeps/ieee754/dbl-64/s_getpayload.c
+++ b/sysdeps/ieee754/dbl-64/s_getpayload.c
@@ -27,6 +27,9 @@ __getpayload (const double *x)
 {
   uint32_t hx, lx;
   EXTRACT_WORDS (hx, lx, *x);
+  if ((hx & 0x7ff00000) != 0x7ff00000
+      || ((hx & 0xfffff) | lx) == 0)
+    return -1;
   hx &= 0x7ffff;
   uint64_t ix = ((uint64_t) hx << 32) | lx;
   if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
diff --git a/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c b/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
index 2c887b93b7..eba96d0c77 100644
--- a/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
+++ b/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
@@ -26,6 +26,9 @@ __getpayload (const double *x)
 {
   uint64_t ix;
   EXTRACT_WORDS64 (ix, *x);
+  if ((ix & 0x7ff0000000000000ULL) != 0x7ff0000000000000ULL
+      || (ix & 0xfffffffffffffULL) == 0)
+    return -1;
   ix &= 0x7ffffffffffffULL;
   return (double) ix;
 }
diff --git a/sysdeps/ieee754/flt-32/s_getpayloadf.c b/sysdeps/ieee754/flt-32/s_getpayloadf.c
index e1d22473ff..1baad4847e 100644
--- a/sysdeps/ieee754/flt-32/s_getpayloadf.c
+++ b/sysdeps/ieee754/flt-32/s_getpayloadf.c
@@ -27,6 +27,9 @@ __getpayloadf (const float *x)
 {
   uint32_t ix;
   GET_FLOAT_WORD (ix, *x);
+  if ((ix & 0x7f800000) != 0x7f800000
+      || (ix & 0x7fffff) == 0)
+    return -1;
   ix &= 0x3fffff;
   if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
     return 0.0f;
diff --git a/sysdeps/ieee754/ldbl-128/s_getpayloadl.c b/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
index db55a00f66..1fbe704608 100644
--- a/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
+++ b/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
@@ -26,6 +26,9 @@ __getpayloadl (const _Float128 *x)
 {
   uint64_t hx, lx;
   GET_LDOUBLE_WORDS64 (hx, lx, *x);
+  if ((hx & 0x7fff000000000000ULL) != 0x7fff000000000000ULL
+      || ((hx & 0xffffffffffffULL) | lx) == 0)
+    return -1;
   hx &= 0x7fffffffffffULL;
   /* Construct the representation of the return value directly, since
      128-bit integers may not be available.  */
diff --git a/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c b/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
index 3b5a1a8414..abbd694eea 100644
--- a/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
+++ b/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
@@ -27,6 +27,9 @@ __getpayloadl (const long double *x)
   double xhi = ldbl_high (*x);
   uint64_t ix;
   EXTRACT_WORDS64 (ix, xhi);
+  if ((ix & 0x7ff0000000000000ULL) != 0x7ff0000000000000ULL
+      || (ix & 0xfffffffffffffULL) == 0)
+    return -1;
   ix &= 0x7ffffffffffffULL;
   if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
     return 0.0L;
diff --git a/sysdeps/ieee754/ldbl-96/s_getpayloadl.c b/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
index 8f09cb74c8..761bd69b58 100644
--- a/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
+++ b/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
@@ -27,6 +27,9 @@ __getpayloadl (const long double *x)
   uint16_t se __attribute__ ((unused));
   uint32_t hx, lx;
   GET_LDOUBLE_WORDS (se, hx, lx, *x);
+  if ((se & 0x7fff) != 0x7fff
+      || ((hx & 0x7fffffff) | lx) == 0)
+    return -1;
   hx &= 0x3fffffff;
   uint64_t ix = ((uint64_t) hx << 32) | lx;
   return (long double) ix;

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Use C2x return value from getpayload of non-NaN (bug 26073)

Sourceware - libc-alpha mailing list
On 7/1/20 6:05 PM, Joseph Myers wrote:
> In TS 18661-1, getpayload had an unspecified return value for a
> non-NaN argument, while C2x requires the return value -1 in that case.

Reviewed TS 18661-1 F.10.13.1 getpayload and confirmed the result is
unspecified for non-NaN argument case.

Confirmed in N2454 that if *x is not a NaN that the result shall be
-1 (not unspecified as in TS 18661-1).

> This patch implements the return value of -1.  I don't think this is
> worth having a new symbol version that's an alias of the old one,
> although occasionally we do that in such cases where the new function
> semantics are a refinement of the old ones (to avoid programs relying
> on the new semantics running on older glibc versions but not behaving
> as intended).
>
> Tested for x86_64 and x86; also ran math/ tests for aarch64 and
> powerpc.

OK for mater.

Reviewed-by: Carlos O'Donell <[hidden email]>

 
> ---
>
> Carlos, is this OK at the current slush development stage?

Yes it is. I consider this a "forward looking" change that while
making a semantic change is within the scope of a bug fix.

> diff --git a/manual/arith.texi b/manual/arith.texi
> index 89c2c064f1..75eaf67fe7 100644
> --- a/manual/arith.texi
> +++ b/manual/arith.texi
> @@ -1895,9 +1895,10 @@ propagated from NaN inputs to the result of a floating-point
>  operation.  These functions, defined by TS 18661-1:2014 and TS
>  18661-3:2015, return the payload of the NaN pointed to by @var{x}
>  (returned as a positive integer, or positive zero, represented as a
> -floating-point number); if @var{x} is not a NaN, they return an
> -unspecified value.  They raise no floating-point exceptions even for
> -signaling NaNs.
> +floating-point number); if @var{x} is not a NaN, they return
> +@minus{}1.  They raise no floating-point exceptions even for signaling
> +NaNs.  (The return value of @minus{}1 for an argument that is not a
> +NaN is specified in C2x; the value was unspecified in TS 18661.)

OK.

>  @end deftypefun
>  
>  @deftypefun int setpayload (double *@var{x}, double @var{payload})
> diff --git a/math/libm-test-getpayload.inc b/math/libm-test-getpayload.inc
> index bd660471aa..72e3c19d1e 100644
> --- a/math/libm-test-getpayload.inc
> +++ b/math/libm-test-getpayload.inc
> @@ -20,17 +20,17 @@
>  
>  static const struct test_f_f_data getpayload_test_data[] =
>    {
> -    TEST_fp_f (getpayload, plus_infty, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, minus_infty, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, plus_zero, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, minus_zero, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, 1000, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, max_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, -max_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, min_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, -min_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, min_subnorm_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> -    TEST_fp_f (getpayload, -min_subnorm_value, IGNORE, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, plus_infty, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, minus_infty, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, plus_zero, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, minus_zero, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, 1000, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, max_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, -max_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, min_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, -min_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, min_subnorm_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> +    TEST_fp_f (getpayload, -min_subnorm_value, -1.0, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),

OK. Add test for variants.

>  #if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
>      TEST_fp_f (getpayload, snan_value_pl ("0x0"), plus_zero, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
>      TEST_fp_f (getpayload, -snan_value_pl ("0x0"), plus_zero, NO_INEXACT_EXCEPTION|ERRNO_UNCHANGED),
> diff --git a/sysdeps/ieee754/dbl-64/s_getpayload.c b/sysdeps/ieee754/dbl-64/s_getpayload.c
> index 3ab89ddd66..5a055be35a 100644
> --- a/sysdeps/ieee754/dbl-64/s_getpayload.c
> +++ b/sysdeps/ieee754/dbl-64/s_getpayload.c
> @@ -27,6 +27,9 @@ __getpayload (const double *x)
>  {
>    uint32_t hx, lx;
>    EXTRACT_WORDS (hx, lx, *x);
> +  if ((hx & 0x7ff00000) != 0x7ff00000
> +      || ((hx & 0xfffff) | lx) == 0)
> +    return -1;

OK.

>    hx &= 0x7ffff;
>    uint64_t ix = ((uint64_t) hx << 32) | lx;
>    if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
> diff --git a/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c b/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
> index 2c887b93b7..eba96d0c77 100644
> --- a/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
> +++ b/sysdeps/ieee754/dbl-64/wordsize-64/s_getpayload.c
> @@ -26,6 +26,9 @@ __getpayload (const double *x)
>  {
>    uint64_t ix;
>    EXTRACT_WORDS64 (ix, *x);
> +  if ((ix & 0x7ff0000000000000ULL) != 0x7ff0000000000000ULL
> +      || (ix & 0xfffffffffffffULL) == 0)
> +    return -1;

OK.

>    ix &= 0x7ffffffffffffULL;
>    return (double) ix;
>  }
> diff --git a/sysdeps/ieee754/flt-32/s_getpayloadf.c b/sysdeps/ieee754/flt-32/s_getpayloadf.c
> index e1d22473ff..1baad4847e 100644
> --- a/sysdeps/ieee754/flt-32/s_getpayloadf.c
> +++ b/sysdeps/ieee754/flt-32/s_getpayloadf.c
> @@ -27,6 +27,9 @@ __getpayloadf (const float *x)
>  {
>    uint32_t ix;
>    GET_FLOAT_WORD (ix, *x);
> +  if ((ix & 0x7f800000) != 0x7f800000
> +      || (ix & 0x7fffff) == 0)
> +    return -1;

OK.

>    ix &= 0x3fffff;
>    if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
>      return 0.0f;
> diff --git a/sysdeps/ieee754/ldbl-128/s_getpayloadl.c b/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
> index db55a00f66..1fbe704608 100644
> --- a/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
> +++ b/sysdeps/ieee754/ldbl-128/s_getpayloadl.c
> @@ -26,6 +26,9 @@ __getpayloadl (const _Float128 *x)
>  {
>    uint64_t hx, lx;
>    GET_LDOUBLE_WORDS64 (hx, lx, *x);
> +  if ((hx & 0x7fff000000000000ULL) != 0x7fff000000000000ULL
> +      || ((hx & 0xffffffffffffULL) | lx) == 0)
> +    return -1;

OK.

>    hx &= 0x7fffffffffffULL;
>    /* Construct the representation of the return value directly, since
>       128-bit integers may not be available.  */
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c b/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
> index 3b5a1a8414..abbd694eea 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_getpayloadl.c
> @@ -27,6 +27,9 @@ __getpayloadl (const long double *x)
>    double xhi = ldbl_high (*x);
>    uint64_t ix;
>    EXTRACT_WORDS64 (ix, xhi);
> +  if ((ix & 0x7ff0000000000000ULL) != 0x7ff0000000000000ULL
> +      || (ix & 0xfffffffffffffULL) == 0)
> +    return -1;

OK.

>    ix &= 0x7ffffffffffffULL;
>    if (FIX_INT_FP_CONVERT_ZERO && ix == 0)
>      return 0.0L;
> diff --git a/sysdeps/ieee754/ldbl-96/s_getpayloadl.c b/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
> index 8f09cb74c8..761bd69b58 100644
> --- a/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
> +++ b/sysdeps/ieee754/ldbl-96/s_getpayloadl.c
> @@ -27,6 +27,9 @@ __getpayloadl (const long double *x)
>    uint16_t se __attribute__ ((unused));
>    uint32_t hx, lx;
>    GET_LDOUBLE_WORDS (se, hx, lx, *x);
> +  if ((se & 0x7fff) != 0x7fff
> +      || ((hx & 0x7fffffff) | lx) == 0)
> +    return -1;

OK.

>    hx &= 0x3fffffff;
>    uint64_t ix = ((uint64_t) hx << 32) | lx;
>    return (long double) ix;
>


--
Cheers,
Carlos.