Re: [PATCH v5] stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 06/10/22 12:01, Letu Ren via Libc-alpha 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.
> Signed-off-by: Letu Ren <fantasquex@gmail.com>

The fix-float-double-convert-nan.h misses the Copyright headers, otherwise
LGTM.

> ---
> I'm not sure about copyright of the new file I created and the name of
> the new function. Please give me some advice. Thanks.
> 
>  stdlib/strfrom-skeleton.c                     |  3 ++-
>  .../generic/fix-float-double-convert-nan.h    | 15 +++++++++++++
>  .../riscv/rvd/fix-float-double-convert-nan.h  | 21 +++++++++++++++++++
>  3 files changed, 38 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..0949dda102
> --- /dev/null
> +++ b/sysdeps/generic/fix-float-double-convert-nan.h
> @@ -0,0 +1,15 @@
> +/* Fix for conversion of float NAN to double. Generic version. */
> +

It is missing the usual Copyright as for other fix-* files.

> +#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..59806052a8
> --- /dev/null
> +++ b/sysdeps/riscv/rvd/fix-float-double-convert-nan.h
> @@ -0,0 +1,21 @@
> +/* Fix for conversion of float NAN to double. RISC-V version. */
> +
> +#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