public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)
@ 2022-10-21 14:54 Letu Ren
  2022-10-21 17:10 ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 5+ messages in thread
From: Letu Ren @ 2022-10-21 14:54 UTC (permalink / raw)
  To: libc-alpha; +Cc: fweimer, joseph, schwab, adhemerval.zanella, Letu Ren

According to the specification of ISO/IEC TS 18661-1:2014,

The strfromd, strfromf, and strfroml functions are equivalent to
snprintf(s, n, format, fp) (7.21.6.5), except the format string contains only
the character %, an optional precision that does not contain an asterisk *, and
one of the conversion specifiers a, A, e, E, f, F, g, or G, which applies to
the type (double, float, or long double) indicated by the function suffix
(rather than  by a length modifier). Use of these functions with any other 20
format string results in undefined behavior.

strfromf will convert the arguement with type float to double first.

According to the latest version of IEEE754 which is published in 2019,

Conversion of a quiet NaN from a narrower format to a wider format in the same
radix, and then back to the same narrower format, should not change the quiet
NaN payload in any way except to make it canonical.

When either an input or result is a NaN, this standard does not interpret the
sign of a NaN. However, operations on bit strings—copy, negate, abs,
copySign—specify the sign bit of a NaN result, sometimes based upon the sign
bit of a NaN operand. The logical predicates totalOrder and isSignMinus are
also affected by the sign bit of a NaN operand. For all other operations, this
standard does not specify the sign bit of a NaN result, even when there is only
one input NaN, or when the NaN is produced from an invalid operation.

converting NAN or -NAN with type float to double doesn't need to keep
the signbit. As a result, this test case isn't mandatory.

The problem is that according to RISC-V ISA manual in chapter 11.3 of
riscv-isa-20191213,

Except when otherwise stated, if the result of a floating-point operation is
NaN, it is the canonical NaN. The canonical NaN has a positive sign and all
significand bits clear except the MSB, a.k.a. the quiet bit. For
single-precision floating-point, this corresponds to the pattern 0x7fc00000.

which means that conversion -NAN from float to double won't keep the signbit.

Since glibc ought to be consistent here between types and architectures, this
patch adds copysign to fix this problem if the string is NAN. This patch
adds two different functions under sysdeps directory to work around the
issue.

This patch has been tested on x86_64 and riscv64.

Resolves: BZ #29501

v2: Change from macros to different inline functions.
v3: Add unlikely check to isnan.
v4: Fix wrong commit message header.
v5: Fix style: add space before parentheses.
v6: Add copyright.
Signed-off-by: Letu Ren <fantasquex@gmail.com>
---
 stdlib/strfrom-skeleton.c                     |  3 +-
 .../generic/fix-float-double-convert-nan.h    | 31 ++++++++++++++++
 .../riscv/rvd/fix-float-double-convert-nan.h  | 37 +++++++++++++++++++
 3 files changed, 70 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/generic/fix-float-double-convert-nan.h
 create mode 100644 sysdeps/riscv/rvd/fix-float-double-convert-nan.h

diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c
index 1fba04bf6a..36e9adcad5 100644
--- a/stdlib/strfrom-skeleton.c
+++ b/stdlib/strfrom-skeleton.c
@@ -27,6 +27,7 @@
 #include <printf.h>
 #include <string.h>
 #include <locale/localeinfo.h>
+#include <fix-float-double-convert-nan.h>
 
 #define UCHAR_T char
 #define L_(Str) Str
@@ -61,7 +62,7 @@ STRFROM (char *dest, size_t size, const char *format, FLOAT f)
      because __printf_fp and __printf_fphex only accept double and long double
      as the floating-point argument.  */
   if (__builtin_types_compatible_p (FLOAT, float))
-    fpnum.flt = f;
+    fpnum.flt = keep_sign_conversion (f);
   else
     fpnum.value = f;
 
diff --git a/sysdeps/generic/fix-float-double-convert-nan.h b/sysdeps/generic/fix-float-double-convert-nan.h
new file mode 100644
index 0000000000..a42c3d7363
--- /dev/null
+++ b/sysdeps/generic/fix-float-double-convert-nan.h
@@ -0,0 +1,31 @@
+/* Fix for conversion of float NAN to double. Generic version.
+   Copyright (C) 2022 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/>.  */
+
+#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H
+#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H
+
+/* This function aims to work around conversions of float -NAN
+   to double returning NAN instead of the correct -NAN in some
+   architectures. */
+static inline double __attribute__ ((always_inline))
+keep_sign_conversion (float flt)
+{
+  return flt;
+}
+
+#endif
diff --git a/sysdeps/riscv/rvd/fix-float-double-convert-nan.h b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h
new file mode 100644
index 0000000000..0d2d871a70
--- /dev/null
+++ b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h
@@ -0,0 +1,37 @@
+/* Fix for conversion of float NAN to double. RISC-V version..
+   Copyright (C) 2022 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/>.  */
+
+#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H
+#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H
+
+#include <math.h>
+
+/* RISC-V rvd instructions do not preserve the signbit of NAN
+   when converting from float to double. */
+static inline double
+keep_sign_conversion (float flt)
+{
+  if (__glibc_unlikely (isnan (flt)))
+  {
+    float x = copysignf (1.f, flt);
+    return copysign ((double) flt, (double) x);
+  }
+  return flt;
+}
+
+#endif
-- 
2.38.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)
  2022-10-21 14:54 [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501) Letu Ren
@ 2022-10-21 17:10 ` Adhemerval Zanella Netto
  2022-10-22  4:43   ` Letu Ren
  2022-10-28 14:26   ` Letu Ren
  0 siblings, 2 replies; 5+ messages in thread
From: Adhemerval Zanella Netto @ 2022-10-21 17:10 UTC (permalink / raw)
  To: Letu Ren, libc-alpha; +Cc: fweimer, joseph, schwab



On 21/10/22 11:54, Letu Ren wrote:
> According to the specification of ISO/IEC TS 18661-1:2014,
> 
> The strfromd, strfromf, and strfroml functions are equivalent to
> snprintf(s, n, format, fp) (7.21.6.5), except the format string contains only
> the character %, an optional precision that does not contain an asterisk *, and
> one of the conversion specifiers a, A, e, E, f, F, g, or G, which applies to
> the type (double, float, or long double) indicated by the function suffix
> (rather than  by a length modifier). Use of these functions with any other 20
> format string results in undefined behavior.
> 
> strfromf will convert the arguement with type float to double first.
> 
> According to the latest version of IEEE754 which is published in 2019,
> 
> Conversion of a quiet NaN from a narrower format to a wider format in the same
> radix, and then back to the same narrower format, should not change the quiet
> NaN payload in any way except to make it canonical.
> 
> When either an input or result is a NaN, this standard does not interpret the
> sign of a NaN. However, operations on bit strings—copy, negate, abs,
> copySign—specify the sign bit of a NaN result, sometimes based upon the sign
> bit of a NaN operand. The logical predicates totalOrder and isSignMinus are
> also affected by the sign bit of a NaN operand. For all other operations, this
> standard does not specify the sign bit of a NaN result, even when there is only
> one input NaN, or when the NaN is produced from an invalid operation.
> 
> converting NAN or -NAN with type float to double doesn't need to keep
> the signbit. As a result, this test case isn't mandatory.
> 
> The problem is that according to RISC-V ISA manual in chapter 11.3 of
> riscv-isa-20191213,
> 
> Except when otherwise stated, if the result of a floating-point operation is
> NaN, it is the canonical NaN. The canonical NaN has a positive sign and all
> significand bits clear except the MSB, a.k.a. the quiet bit. For
> single-precision floating-point, this corresponds to the pattern 0x7fc00000.
> 
> which means that conversion -NAN from float to double won't keep the signbit.
> 
> Since glibc ought to be consistent here between types and architectures, this
> patch adds copysign to fix this problem if the string is NAN. This patch
> adds two different functions under sysdeps directory to work around the
> issue.
> 
> This patch has been tested on x86_64 and riscv64.
> 
> Resolves: BZ #29501
> 
> v2: Change from macros to different inline functions.
> v3: Add unlikely check to isnan.
> v4: Fix wrong commit message header.
> v5: Fix style: add space before parentheses.
> v6: Add copyright.
> Signed-off-by: Letu Ren <fantasquex@gmail.com>

LGTM, thanks.  Really minor nits below that I did not catch before, there is no 
need to send a v7 (just fix it before install).

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  stdlib/strfrom-skeleton.c                     |  3 +-
>  .../generic/fix-float-double-convert-nan.h    | 31 ++++++++++++++++
>  .../riscv/rvd/fix-float-double-convert-nan.h  | 37 +++++++++++++++++++
>  3 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 sysdeps/generic/fix-float-double-convert-nan.h
>  create mode 100644 sysdeps/riscv/rvd/fix-float-double-convert-nan.h
> 
> diff --git a/stdlib/strfrom-skeleton.c b/stdlib/strfrom-skeleton.c
> index 1fba04bf6a..36e9adcad5 100644
> --- a/stdlib/strfrom-skeleton.c
> +++ b/stdlib/strfrom-skeleton.c
> @@ -27,6 +27,7 @@
>  #include <printf.h>
>  #include <string.h>
>  #include <locale/localeinfo.h>
> +#include <fix-float-double-convert-nan.h>
>  
>  #define UCHAR_T char
>  #define L_(Str) Str
> @@ -61,7 +62,7 @@ STRFROM (char *dest, size_t size, const char *format, FLOAT f)
>       because __printf_fp and __printf_fphex only accept double and long double
>       as the floating-point argument.  */
>    if (__builtin_types_compatible_p (FLOAT, float))
> -    fpnum.flt = f;
> +    fpnum.flt = keep_sign_conversion (f);
>    else
>      fpnum.value = f;
>  
> diff --git a/sysdeps/generic/fix-float-double-convert-nan.h b/sysdeps/generic/fix-float-double-convert-nan.h
> new file mode 100644
> index 0000000000..a42c3d7363
> --- /dev/null
> +++ b/sysdeps/generic/fix-float-double-convert-nan.h
> @@ -0,0 +1,31 @@
> +/* Fix for conversion of float NAN to double. Generic version.

Double space after period.

> +   Copyright (C) 2022 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/>.  */
> +
> +#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H
> +#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H
> +
> +/* This function aims to work around conversions of float -NAN
> +   to double returning NAN instead of the correct -NAN in some
> +   architectures. */

Double space before close comment.

> +static inline double __attribute__ ((always_inline))
> +keep_sign_conversion (float flt)
> +{
> +  return flt;
> +}
> +
> +#endif
> diff --git a/sysdeps/riscv/rvd/fix-float-double-convert-nan.h b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h
> new file mode 100644
> index 0000000000..0d2d871a70
> --- /dev/null
> +++ b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h
> @@ -0,0 +1,37 @@
> +/* Fix for conversion of float NAN to double. RISC-V version..

Double space after period and remove last final period.

> +   Copyright (C) 2022 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/>.  */
> +
> +#ifndef FIX_FLOAT_DOUBLE_CONVERT_NAN_H
> +#define FIX_FLOAT_DOUBLE_CONVERT_NAN_H
> +
> +#include <math.h>
> +
> +/* RISC-V rvd instructions do not preserve the signbit of NAN
> +   when converting from float to double. */
> +static inline double
> +keep_sign_conversion (float flt)
> +{
> +  if (__glibc_unlikely (isnan (flt)))
> +  {
> +    float x = copysignf (1.f, flt);
> +    return copysign ((double) flt, (double) x);
> +  }
> +  return flt;
> +}
> +
> +#endif

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)
  2022-10-21 17:10 ` Adhemerval Zanella Netto
@ 2022-10-22  4:43   ` Letu Ren
  2022-10-28 14:26   ` Letu Ren
  1 sibling, 0 replies; 5+ messages in thread
From: Letu Ren @ 2022-10-22  4:43 UTC (permalink / raw)
  To: Adhemerval Zanella Netto; +Cc: libc-alpha, fweimer, joseph, schwab

> LGTM, thanks.  Really minor nits below that I did not catch before, there is no
> need to send a v7 (just fix it before install).

Thanks for your review!

> Double space after period.
>
> Double space before close comment.
>
> Double space after period and remove last final period.

I will pay attention to these style problems next time.

Regards
Letu Ren

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)
  2022-10-21 17:10 ` Adhemerval Zanella Netto
  2022-10-22  4:43   ` Letu Ren
@ 2022-10-28 14:26   ` Letu Ren
  2022-10-28 14:32     ` Adhemerval Zanella Netto
  1 sibling, 1 reply; 5+ messages in thread
From: Letu Ren @ 2022-10-28 14:26 UTC (permalink / raw)
  To: Adhemerval Zanella Netto; +Cc: libc-alpha, fweimer, joseph, schwab

Hi,

  Actually, this is a ping email. It's been almost a week since
Adhemerval Zanella reviewed this patch. Hope this patch will be merged
soon.

Regards,
Letu

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)
  2022-10-28 14:26   ` Letu Ren
@ 2022-10-28 14:32     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 5+ messages in thread
From: Adhemerval Zanella Netto @ 2022-10-28 14:32 UTC (permalink / raw)
  To: Letu Ren; +Cc: libc-alpha, fweimer, joseph, schwab



On 28/10/22 11:26, Letu Ren wrote:
> Hi,
> 
>   Actually, this is a ping email. It's been almost a week since
> Adhemerval Zanella reviewed this patch. Hope this patch will be merged
> soon.
> 
> Regards,
> Letu


I will commit this for you.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-10-28 14:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-21 14:54 [PATCH v6] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501) Letu Ren
2022-10-21 17:10 ` Adhemerval Zanella Netto
2022-10-22  4:43   ` Letu Ren
2022-10-28 14:26   ` Letu Ren
2022-10-28 14:32     ` Adhemerval Zanella Netto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).