[PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[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.

This patch changes the original -NAN test case to NAN and adds a special
test which accepts two possible results to test -NAN.

This patch has been tested on x86_64 and riscv64. Except some tests
fails on riscv64 due to timeout, all tests pass.

Signed-off-by: Letu Ren <fantasquex@gmail.com>
---
 stdlib/tst-strfrom-locale.c |  6 +++++-
 stdlib/tst-strfrom.c        |  6 +++++-
 stdlib/tst-strfrom.h        | 26 ++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/stdlib/tst-strfrom-locale.c b/stdlib/tst-strfrom-locale.c
index 85907c0ce1..1b0418c20e 100644
--- a/stdlib/tst-strfrom-locale.c
+++ b/stdlib/tst-strfrom-locale.c
@@ -37,10 +37,14 @@ static const struct test tests[] = {
   TEST ("5,900000e-16", "%e", 50, 12, 5.9e-16),
   TEST ("1,234500e+20", "%e", 50, 12, 12.345e19),
   TEST ("1,000000e+05", "%e", 50, 12, 1e5),
-  TEST ("-NAN", "%G", 50, 4, -NAN_),
+  TEST ("NAN", "%G", 50, 3, NAN_),
   TEST ("-inf", "%g", 50, 4, -INF),
   TEST ("inf", "%g", 50, 3, INF)
 };
+/* Tests with multiple possible results.  */
+static const struct sptest sptests[] = {
+  SPTEST ("-NAN", "NAN", "%G", 50, 4, 3, -NAN_)
+};
 /* Tests with buffer size small.  */
 static const struct test stest[] = {
   TEST ("1234", "%g", 5, 7, 12345.345),
diff --git a/stdlib/tst-strfrom.c b/stdlib/tst-strfrom.c
index 3a447eac12..3186db86fd 100644
--- a/stdlib/tst-strfrom.c
+++ b/stdlib/tst-strfrom.c
@@ -37,10 +37,14 @@ static const struct test tests[] = {
   TEST ("5.900000e-16", "%e", 50, 12, 5.9e-16),
   TEST ("1.234500e+20", "%e", 50, 12, 12.345e19),
   TEST ("1.000000e+05", "%e", 50, 12, 1e5),
-  TEST ("-NAN", "%G", 50, 4, -NAN_),
+  TEST ("NAN", "%G", 50, 3, NAN_),
   TEST ("-inf", "%g", 50, 4, -INF),
   TEST ("inf", "%g", 50, 3, INF)
 };
+/* Tests with multiple possible results.  */
+static const struct sptest sptests[] = {
+  SPTEST ("-NAN", "NAN", "%G", 50, 4, 3, -NAN_)
+};
 /* Tests with buffer size small.  */
 static const struct test stest[] = {
   TEST ("1234", "%g", 5, 7, 12345.345),
diff --git a/stdlib/tst-strfrom.h b/stdlib/tst-strfrom.h
index 2820a580c2..e80027fde9 100644
--- a/stdlib/tst-strfrom.h
+++ b/stdlib/tst-strfrom.h
@@ -59,6 +59,20 @@ struct test {
   {								\
     s, fmt, size, rc, { GEN_TEST_STRTOD_FOREACH (ENTRY, val) }	\
   }
+/* Tests with multiple possible results.  */
+struct sptest {
+  const char *s0;
+  const char *s1;
+  const char *fmt;
+  int size;
+  int rc0;
+  int rc1;
+  struct test_input t;
+};
+#define SPTEST(s0, s1, fmt, size, rc0, rc1, val)				\
+  {								\
+    s0, s1, fmt, size, rc0, rc1, { GEN_TEST_STRTOD_FOREACH (ENTRY, val) }	\
+  }
 /* Hexadecimal tests.  */
 struct htests
 {
@@ -100,6 +114,18 @@ test_ ## FSUF (void)							\
 	  status++;							\
 	}								\
     }									\
+  for (i = 0; i < sizeof (sptests) / sizeof (sptests[0]); i++)		\
+    {									\
+      rc = FTOSTR (buf, sptests[i].size, sptests[i].fmt, sptests[i].t.FSUF);	\
+      rc1 = (strcmp (buf, sptests[i].s0) != 0) || (rc != sptests[i].rc0);	\
+      rc1 &= (strcmp (buf, sptests[i].s1) != 0) || (rc != sptests[i].rc1);	\
+      if (rc1)								\
+	{								\
+	  printf (#FTOSTR ": got %s (%d), expected %s (%d) or %s (%d)\n",		\
+		  buf, rc, sptests[i].s0, sptests[i].rc0, sptests[i].s1, sptests[i].rc1); \
+	  status++;							\
+	}								\
+    }									\
   for (i = 0; i < sizeof (htest) / sizeof (htest[0]); i++)		\
     {									\
       rc = FTOSTR (buf, 50, htest[i].fmt, htest[i].t.FSUF);		\
-- 
2.37.3

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Joseph Myers]

On Sat, 17 Sep 2022, Letu Ren via Libc-alpha wrote:

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

The *current implementation* will.  That's not part of the specification, 
it's a quality-of-implementation bug (IEEE 754 leaves the presence of a 
sign for a NaN in the results of convertToDecimalCharacter and 
convertToHexCharacter optional, but glibc ought to be consistent here 
between types and architectures).  That bug should be fixed; this patch 
should not be applied.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Florian Weimer]

* Joseph Myers:

> On Sat, 17 Sep 2022, Letu Ren via Libc-alpha wrote:
>
>> strfromf will convert the arguement with type float to double first.
>
> The *current implementation* will.  That's not part of the specification, 
> it's a quality-of-implementation bug (IEEE 754 leaves the presence of a 
> sign for a NaN in the results of convertToDecimalCharacter and 
> convertToHexCharacter optional, but glibc ought to be consistent here 
> between types and architectures).  That bug should be fixed; this patch 
> should not be applied.

I think the conversion to double is required because according to the
quote from ISO/IEC TS 18661-1:2014 in the bug, strfromf is defined in
terms of snprintf, and calling snprintf promotes float arguments that do
not correspond to a declared parameter to double as part of the default
argument promotions.

(Admittedly, this could be a bug in the TS.)

Thanks,
Florian

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Letu Ren]

On Sun, Sep 18, 2022 at 4:31 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> I think the conversion to double is required because according to the
> quote from ISO/IEC TS 18661-1:2014 in the bug, strfromf is defined in
> terms of snprintf, and calling snprintf promotes float arguments that do
> not correspond to a declared parameter to double as part of the default
> argument promotions.
>
> (Admittedly, this could be a bug in the TS.)

I agree with you that the conversion is required. The problem is that those
standards don't define whether we should keep the signbit of -NAN explicitly.
Joseph thinks we should keep the behavior of glibc consistent across
different architectures and keep the signbit explicitly, while I think
the signbit
doesn't really matter originally. Since I'm not familiar with glibc, I
would like to
hear from more people. How do you think this problem should be solved?
Just like what Joseph says or something else?

Thanks,
Letu Ren

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Joseph Myers]

On Sun, 18 Sep 2022, Florian Weimer wrote:

> I think the conversion to double is required because according to the
> quote from ISO/IEC TS 18661-1:2014 in the bug, strfromf is defined in
> terms of snprintf, and calling snprintf promotes float arguments that do
> not correspond to a declared parameter to double as part of the default
> argument promotions.

I don't think reading as literally passed to the variable-arguments 
function snprintf is very helpful, considering that the strfrom functions 
are supported for types that have no corresponding printf format (and 
supporting e.g. strfromf128 is one motivation for having these functions).  
That is, the equivalence to snprintf should be considered to be for the 
semantics of conversion to character strings, rather than for applying 
default argument promotions.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Florian Weimer]

* Joseph Myers:

> On Sun, 18 Sep 2022, Florian Weimer wrote:
>
>> I think the conversion to double is required because according to the
>> quote from ISO/IEC TS 18661-1:2014 in the bug, strfromf is defined in
>> terms of snprintf, and calling snprintf promotes float arguments that do
>> not correspond to a declared parameter to double as part of the default
>> argument promotions.
>
> I don't think reading as literally passed to the variable-arguments 
> function snprintf is very helpful, considering that the strfrom functions 
> are supported for types that have no corresponding printf format (and 
> supporting e.g. strfromf128 is one motivation for having these functions).  
> That is, the equivalence to snprintf should be considered to be for the 
> semantics of conversion to character strings, rather than for applying 
> default argument promotions.

Ah, so a bug in TS.  Can the text still be fixed, or is this impossible
because it's a TS?

Thanks,
Florian

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Letu Ren]

On Mon, Sep 19, 2022 at 3:38 PM Florian Weimer <fweimer@redhat.com> wrote:

> Ah, so a bug in TS.  Can the text still be fixed, or is this impossible
> because it's a TS?

I wonder whether there is any update?

FYI, gawk met the same issue and they pushed a patch to copysign in
all architectures.

http://git.savannah.gnu.org/cgit/gawk.git/commit/?id=a3799ae3f5dd6648040d499224cc6dea61b355dd

Although I really doubt whether it is needed, it is a doable solution.

Thanks,
Letu Ren

Re: [PATCH v2] stdlib/strfrom: Change -NAN test to multiple possible results (bug 29501)

[Joseph Myers]

On Mon, 19 Sep 2022, Florian Weimer via Libc-alpha wrote:

> * Joseph Myers:
> 
> > On Sun, 18 Sep 2022, Florian Weimer wrote:
> >
> >> I think the conversion to double is required because according to the
> >> quote from ISO/IEC TS 18661-1:2014 in the bug, strfromf is defined in
> >> terms of snprintf, and calling snprintf promotes float arguments that do
> >> not correspond to a declared parameter to double as part of the default
> >> argument promotions.
> >
> > I don't think reading as literally passed to the variable-arguments 
> > function snprintf is very helpful, considering that the strfrom functions 
> > are supported for types that have no corresponding printf format (and 
> > supporting e.g. strfromf128 is one motivation for having these functions).  
> > That is, the equivalence to snprintf should be considered to be for the 
> > semantics of conversion to character strings, rather than for applying 
> > default argument promotions.
> 
> Ah, so a bug in TS.  Can the text still be fixed, or is this impossible
> because it's a TS?

I've noted it as something to consider submitting as an NB comment on the 
C2x CD ballot.  TS 18661-1, -2 and -3 will be superseded by C2x (and thus 
hopefully withdrawn at their next systematic review after C2x).  There 
should be subsequent revisions of -4 (the parts not merged into C2x, plus 
new functions from IEEE 754-2019) and -5, updated to apply to C2x.

-- 
Joseph S. Myers
joseph@codesourcery.com