[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[fantasquex at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29501

--- Comment #5 from Letu Ren <fantasquex at gmail dot com> ---
As mentioned in 11.3 section of the latest RISC-V ISA manual 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.

And in section 6.3 of the latest version of IEEE 754 which is published in
2019,

When either an input or result is a NaN, this standard does not interpret the
sign of a NaN.

So, I think the problem is that IEEE 754 doesn't care about the sign bit of
NAN. And RISC-V ISA follows IEEE 754 and sets the default sign bit of NAN to
positive. The test case fails. Maybe we can change the test case a little bit,
since the sign bit of NAN doesn't matter?

BTW, I'm interested in "hook" you mentioned. Which part of code should I take a
look?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[joseph at codesourcery dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29501

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
The documentation of the source tree structure in maint.texi is rather out 
of date, but the basic idea is that you have a header in sysdeps/generic 
that defines an inline function doing a trivial direct conversion from 
float to double and then a version of that header in sysdeps/riscv/rvd 
that makes special use of copysign to preserve the sign of a NaN.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[fantasquex at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29501

--- Comment #7 from Letu Ren <fantasquex at gmail dot com> ---
The root cause is that in function STRFROM of stdlib/strfrom-skeleton.c

```c
/* Single-precision values need to be stored in a double type, because
   __printf_fp_l and __printf_fphex do not accept the float type.  */
union {
  double flt;
  FLOAT value;
} fpnum;
const void *fpptr;
fpptr = &fpnum;

/* Single-precision values need to be converted into double-precision,
   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;
else
  fpnum.value = f;
```


argument f with type float is converted to double in the beginning. On riscv64,
-NAN with type float doesn't keep signbit when it is converted to double. That
is why this testcase only fails with strfromf.

I have confirmed that if the signbit is kept, the testcase will pass.

I'm really curious why the implementation converts the argument to double in
the beginning and look through 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.

Since `snprintf` doesn't consume an argument with type double, there must be a
conversion from float to double. According to spec of IEEE 754 and RISCV-ISA
manual, all these behaviors are reasonable. The signbit of NAN doesn't matter.

I understand what you mean a hook. Get the signbit of the argument, copy the
value and also copy the signbit explicitly. As I mentioned before, all current
behaviors are reasonable according to the spec. I really doubt whether we
should add such a hook to resolve a problem which shouldn't be a problem.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29501

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Adhemerval Zanella
<azanella@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=0cc0033ef19bd3378445c2b851e53d7255cb1b1e

commit 0cc0033ef19bd3378445c2b851e53d7255cb1b1e
Author: Letu Ren <fantasquex@gmail.com>
Date:   Fri Oct 21 22:54:50 2022 +0800

    stdlib/strfrom: Add copysign to fix NAN issue on riscv (BZ #29501)

    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>
    Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/29501] Check failed on stdlib/tst-strfrom while building for RISCV64 on a unmatched hardware

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29501

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |2.37
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #9 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Fixed on 2.37.

-- 
You are receiving this mail because:
You are on the CC list for the bug.