strtod ("nan") returns negative NaN

Message ID 20180815.000539.1012490218433540835.trueroad@trueroad.jp
State New
Headers show
Series
  • strtod ("nan") returns negative NaN
Related show

Commit Message

Masamichi Hosoda Aug. 14, 2018, 3:05 p.m.
Hi

I've found that strtod ("nan") returns negative NaN on Cygwin 64 bit.
https://cygwin.com/ml/cygwin/2018-08/msg00168.html

On Linux with glibc, both strtod ("nan")
and strtod ("-nan") return positive NaN.

So I've created the patch that behaves like glibc.
Both strtod ("nan") and strtod ("-nan") return positive NaN.

Sample code:
```
#include <stdio.h>
#include <stdlib.h>

int main (void)
{
  printf ("strtof (\"nan\", NULL) = %f\n", strtof ("nan", NULL));
  printf ("strtof (\"-nan\", NULL) = %f\n", strtof ("-nan", NULL));
  printf ("strtod (\"nan\", NULL) = %f\n", strtod ("nan", NULL));
  printf ("strtod (\"-nan\", NULL) = %f\n", strtod ("-nan", NULL));
  printf ("strtold (\"nan\", NULL) = %Lf\n", strtold ("nan", NULL));
  printf ("strtold (\"-nan\", NULL) = %Lf\n", strtold ("-nan", NULL));
}
```

The result of Cygwin (newlib) without my patch:
```
strtof ("nan", NULL) = nan
strtof ("-nan", NULL) = nan
strtod ("nan", NULL) = -nan
strtod ("-nan", NULL) = nan
strtold ("nan", NULL) = -nan
strtold ("-nan", NULL) = -nan
```

The result of Linux (glibc, Ubuntu 16.04):
```
strtof ("nan", NULL) = nan
strtof ("-nan", NULL) = nan
strtod ("nan", NULL) = nan
strtod ("-nan", NULL) = nan
strtold ("nan", NULL) = nan
strtold ("-nan", NULL) = nan
```

The result of FreeBSD 10.1 (BSD libc):
```
strtof ("nan", NULL) = nan
strtof ("-nan", NULL) = nan
strtod ("nan", NULL) = nan
strtod ("-nan", NULL) = nan
strtold ("nan", NULL) = nan
strtold ("-nan", NULL) = nan
```

The result of Cygwin (newlib) with my patch:
```
strtof ("nan", NULL) = nan
strtof ("-nan", NULL) = nan
strtod ("nan", NULL) = nan
strtod ("-nan", NULL) = nan
strtold ("nan", NULL) = nan
strtold ("-nan", NULL) = nan
```

Thanks.

Comments

Corinna Vinschen Aug. 14, 2018, 4:01 p.m. | #1
On Aug 15 00:05, Masamichi Hosoda wrote:
> Hi

> 

> I've found that strtod ("nan") returns negative NaN on Cygwin 64 bit.

> https://cygwin.com/ml/cygwin/2018-08/msg00168.html

> 

> On Linux with glibc, both strtod ("nan")

> and strtod ("-nan") return positive NaN.

> 

> So I've created the patch that behaves like glibc.

> Both strtod ("nan") and strtod ("-nan") return positive NaN.


Looks good on Cygwin.  Any comments from non-Cygwin devs?
If nobody has a problem I push the patch in the next days.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Craig Howland Aug. 14, 2018, 5:43 p.m. | #2
On 08/14/2018 11:05 AM, Masamichi Hosoda wrote:
> Hi

>

> I've found that strtod ("nan") returns negative NaN on Cygwin 64 bit.

> https://cygwin.com/ml/cygwin/2018-08/msg00168.html

>

> On Linux with glibc, both strtod ("nan")

> and strtod ("-nan") return positive NaN.

>

> So I've created the patch that behaves like glibc.

> Both strtod ("nan") and strtod ("-nan") return positive NaN.

>

> Sample code:

> ```

> #include <stdio.h>

> #include <stdlib.h>

>

> int main (void)

> {

>    printf ("strtof (\"nan\", NULL) = %f\n", strtof ("nan", NULL));

>    printf ("strtof (\"-nan\", NULL) = %f\n", strtof ("-nan", NULL));

>    printf ("strtod (\"nan\", NULL) = %f\n", strtod ("nan", NULL));

>    printf ("strtod (\"-nan\", NULL) = %f\n", strtod ("-nan", NULL));

>    printf ("strtold (\"nan\", NULL) = %Lf\n", strtold ("nan", NULL));

>    printf ("strtold (\"-nan\", NULL) = %Lf\n", strtold ("-nan", NULL));

> }

> ```

>

> The result of Cygwin (newlib) without my patch:

> ```

> strtof ("nan", NULL) = nan

> strtof ("-nan", NULL) = nan

> strtod ("nan", NULL) = -nan

> strtod ("-nan", NULL) = nan

> strtold ("nan", NULL) = -nan

> strtold ("-nan", NULL) = -nan

> ```

> ...

>

> Thanks.

The Newlib math functions generally do support signed NANs.  The C standard says 
(in section 5.2.4.2.2 Characteristics of floating types <float.h>) NANs can be 
treated as having a sign or not.  But since the math library does support signed 
NANs, the proper behavior for the strtox() functions would be to listen to the 
sign rather than ignoring it.  So while the current Cygwin 64 behavior is 
clearly incorrect, the given patch needs to be adjusted to listen to sign.

(It can be seen that the math functions support signed NANs by looking, for 
example, at s_copysign.c.  it blindly copies the sign without checking the type 
of the arguments.  The standard requires requests to set sign to be ignored when 
NANs are not treated as having signs.)

I cannot generate git diffs at this time (corporate firewall), but here is how I 
think strtof() would need to be fixed after strtod() is:

$ diff -pu strtod.c.20180420 strtod.c
--- strtod.c.20180420    2018-04-20 10:32:36.974479745 -0400
+++ strtod.c    2018-08-14 13:27:57.958177691 -0400
@@ -1285,7 +1285,7 @@ strtof_l (const char *__restrict s00, ch
  {
    double val = _strtod_l (_REENT, s00, se, loc);
    if (isnan (val))
-    return nanf (NULL);
+    return copysign( nanf (NULL), signbit(val) ? -1.0f : 1.0f);
    float retval = (float) val;
  #ifndef NO_ERRNO
    if (isinf (retval) && !isinf (val))
@@ -1300,7 +1300,7 @@ strtof (const char *__restrict s00,
  {
    double val = _strtod_l (_REENT, s00, se, __get_current_locale ());
    if (isnan (val))
-    return nanf (NULL);
+    return copysign( nanf (NULL), signbit(val) ? -1.0f : 1.0f);
    float retval = (float) val;
  #ifndef NO_ERRNO
    if (isinf (retval) && !isinf (val))


By the way, here are the results of the test prints from an embedded 64-bit ARM:

strtof ("nan", NULL) = 0.000000
strtof ("-nan", NULL) = 0.000000
strtod ("nan", NULL) = nan
strtod ("-nan", NULL) = nan
strtold ("nan", NULL) = inf
strtold ("-nan", NULL) = 2315841784746322880031071374419981810209660594968745666
57923565917084799991808.000000

Craig
Joseph Myers Aug. 14, 2018, 7:44 p.m. | #3
On Wed, 15 Aug 2018, Masamichi Hosoda wrote:

> On Linux with glibc, both strtod ("nan")

> and strtod ("-nan") return positive NaN.

> 

> So I've created the patch that behaves like glibc.

> Both strtod ("nan") and strtod ("-nan") return positive NaN.


I would suggest that you should not consider fixed bugs in glibc (bug 
23007 in this case) to be appropriate to emulate in other libraries.

-- 
Joseph S. Myers
joseph@codesourcery.com
Masamichi Hosoda Aug. 15, 2018, 12:02 a.m. | #4
> On Wed, 15 Aug 2018, Masamichi Hosoda wrote:

> 

>> On Linux with glibc, both strtod ("nan")

>> and strtod ("-nan") return positive NaN.

>> 

>> So I've created the patch that behaves like glibc.

>> Both strtod ("nan") and strtod ("-nan") return positive NaN.

> 

> I would suggest that you should not consider fixed bugs in glibc (bug 

> 23007 in this case) to be appropriate to emulate in other libraries.


I've create the patch that behaves to preserve the sign bit.

The result on Cygwin 64 bit (newlib, x86_64) with the patch:
```
strtof ("nan", NULL) = nan
strtof ("-nan", NULL) = -nan
strtod ("nan", NULL) = nan
strtod ("-nan", NULL) = -nan
strtold ("nan", NULL) = nan
strtold ("-nan", NULL) = -nan
```

Thank you for your suggestion.
From 91cf4a20e0773f4a38d6d56b0867fe3725859e5e Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Tue, 14 Aug 2018 22:29:34 +0900
Subject: [PATCH v2 1/2] Fix strtod ("nan") returns negative NaN

The definition of qNaN for x86_64 and i386 was wrong.
So strtod ("nan") and strtold ("nan") returned negative NaN
instead of positive NaN.

strtof ("nan") returns positive NaN so it does not have this issue.

This commit fixes definition of qNaN for x86_64 and i386.
So strtod ("nan") and strtold ("nan") return positive NaN.
---
 newlib/libc/stdlib/gd_qnan.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/newlib/libc/stdlib/gd_qnan.h b/newlib/libc/stdlib/gd_qnan.h
index b775f82..8b0726a 100644
--- a/newlib/libc/stdlib/gd_qnan.h
+++ b/newlib/libc/stdlib/gd_qnan.h
@@ -26,6 +26,20 @@
 #elif defined(__IEEE_LITTLE_ENDIAN)
 
 #if !defined(__mips)
+#if defined (__x86_64__) || defined (__i386__)
+#define f_QNAN 0x7fc00000
+#define d_QNAN0 0x0
+#define d_QNAN1 0x7ff80000
+#define ld_QNAN0 0x0
+#define ld_QNAN1 0xc0000000
+#define ld_QNAN2 0x7fff
+#define ld_QNAN3 0x0
+#define ldus_QNAN0 0x0
+#define ldus_QNAN1 0x0
+#define ldus_QNAN2 0x0
+#define ldus_QNAN3 0xc000
+#define ldus_QNAN4 0x7fff
+#else
 #define f_QNAN 0xffc00000
 #define d_QNAN0 0x0
 #define d_QNAN1 0xfff80000
@@ -38,6 +52,7 @@
 #define ldus_QNAN2 0x0
 #define ldus_QNAN3 0xc000
 #define ldus_QNAN4 0xffff
+#endif
 #elif defined(__mips_nan2008)
 #define f_QNAN 0x7fc00000
 #define d_QNAN0 0x0
-- 
2.17.0
From 51363ce08ffc587b206f2efdd72527ffba7b4381 Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Wed, 15 Aug 2018 08:39:22 +0900
Subject: [PATCH v2 2/2] Fix strtof ("-nan") returns positive NaN

strtof ("-nan") returned positive NaN instead of negative NaN.
strtod ("-nan") and strtold ("-nan") return negative NaN.

Linux glibc has been fixed
that strto{f|d|ld} ("-nan") returns negative NaN.
https://sourceware.org/bugzilla/show_bug.cgi?id=23007

This commit makes strtof preserves the negative sign bit
when parsing "-nan" like glibc.
---
 newlib/libc/stdlib/strtod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 0cfa9e6..9c5ed56 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -1285,7 +1285,7 @@ strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)
 {
   double val = _strtod_l (_REENT, s00, se, loc);
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
@@ -1300,7 +1300,7 @@ strtof (const char *__restrict s00,
 {
   double val = _strtod_l (_REENT, s00, se, __get_current_locale ());
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
-- 
2.17.0
Masamichi Hosoda Aug. 15, 2018, 12:32 a.m. | #5
[...]
> By the way, here are the results of the test prints from an embedded

> 64-bit ARM:

> 

> strtof ("nan", NULL) = 0.000000

> strtof ("-nan", NULL) = 0.000000

> strtod ("nan", NULL) = nan

> strtod ("-nan", NULL) = nan

> strtold ("nan", NULL) = inf

> strtold ("-nan", NULL) =

> 2315841784746322880031071374419981810209660594968745666

> 57923565917084799991808.000000


If I understand correctly, qNaN definition for 64-bit ARM is wrong
like x86_64 and i386 definition was wrong.

The following code can generate the correct definition.

```
/* gcc arch-nans.c */

#include <inttypes.h>
#include <math.h>
#include <stdio.h>
#include <stdint.h>

int main (void)
{
  union
  {
    float f;
    uint32_t u;
  } fb;

  union
  {
    double d;
    uint32_t u[2];
  } db;

  union
  {
    long double ld;
    uint32_t u[4];
    uint16_t us[5];
  } ldb;

  fb.u = 0;
  fb.f = nanf ("");
  printf ("#define f_QNAN 0x%"PRIx32"\n", fb.u);

  db.u[0] = 0;
  db.u[1] = 0;
  db.d = nan ("");
  printf ("#define d_QNAN0 0x%"PRIx32"\n"
	  "#define d_QNAN1  0x%"PRIx32"\n", db.u[0], db.u[1]);

  ldb.u[0] = 0;
  ldb.u[1] = 0;
  ldb.u[2] = 0;
  ldb.u[3] = 0;
  ldb.ld = nanl ("");
  printf ("#define ld_QNAN0 0x%"PRIx32"\n"
	  "#define ld_QNAN1 0x%"PRIx32"\n"
	  "#define ld_QNAN2 0x%"PRIx32"\n"
	  "#define ld_QNAN2 0x%"PRIx32"\n",
	  ldb.u[0], ldb.u[1], ldb.u[2], ldb.u[3]);
  printf ("#define ldus_QNAN0 0x%"PRIx16"\n"
	  "#define ldus_QNAN1 0x%"PRIx16"\n"
	  "#define ldus_QNAN2 0x%"PRIx16"\n"
	  "#define ldus_QNAN3 0x%"PRIx16"\n"
	  "#define ldus_QNAN4 0x%"PRIx16"\n",
	  ldb.us[0], ldb.us[1], ldb.us[2], ldb.us[3], ldb.us[4]);
}
```

The result on Cygwin 64 bit (x86_64) and 32 bit (i386):
```
#define f_QNAN 0x7fc00000
#define d_QNAN0 0x0
#define d_QNAN1  0x7ff80000
#define ld_QNAN0 0x0
#define ld_QNAN1 0xc0000000
#define ld_QNAN2 0x7fff
#define ld_QNAN2 0x0
#define ldus_QNAN0 0x0
#define ldus_QNAN1 0x0
#define ldus_QNAN2 0x0
#define ldus_QNAN3 0xc000
#define ldus_QNAN4 0x7fff
```

My patch
0001-Fix-strtod-nan-returns-negative-NaN.patch
v2-0001-Fix-strtod-nan-returns-negative-NaN.patch
uses the generated definition.
Craig Howland Aug. 15, 2018, 2:24 a.m. | #6
On 08/14/2018 08:02 PM, Masamichi Hosoda wrote:
>> On Wed, 15 Aug 2018, Masamichi Hosoda wrote:

>>

>>> On Linux with glibc, both strtod ("nan")

>>> and strtod ("-nan") return positive NaN.

>>>

>>> So I've created the patch that behaves like glibc.

>>> Both strtod ("nan") and strtod ("-nan") return positive NaN.

>> I would suggest that you should not consider fixed bugs in glibc (bug

>> 23007 in this case) to be appropriate to emulate in other libraries.

> I've create the patch that behaves to preserve the sign bit.

>

> The result on Cygwin 64 bit (newlib, x86_64) with the patch:

> ```

> strtof ("nan", NULL) = nan

> strtof ("-nan", NULL) = -nan

> strtod ("nan", NULL) = nan

> strtod ("-nan", NULL) = -nan

> strtold ("nan", NULL) = nan

> strtold ("-nan", NULL) = -nan

> ```

>

> Thank you for your suggestion.

      The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In 
addition, the d_QNAN* values should be 0x0 and 0x7FF80000, with only the index 
changing based on byte ordering.  So instead of them being inside of a 
x86/x86_64 define, they should just have their values corrected in the 
LITTLE_ENDIAN clause.
      x86 does use a different coding for their 80-bit long double, so the 
ldus_QNAN* defines could belong within an x86 define.  On the other hand, the 
ldus_QNAN* defines only apply for Intel 80-bit, so in that respect don't need to 
be within a guard.
      The ld_QNAN defines are not actually used anywhere.  If they were, 
however, Intel 80-bit would require different values than 128-bit.  However, the 
long double support in Newlib really only works when long double is the same 
size as double.  Some functions can work with Intel 80-bit, but almost none of 
them work with 128-bit.
      Given the prior considerations, I suggest that only the values get fixed 
so that Cygwin works, but the #if x86 is not added.  For this to work on other 
platforms (128-bit long double), more work will be needed.  Again, sorry that I 
can't provide a git diff patch, but here's a suggested one with diff -pu.  It 
contains the value corrections from Masamichi, but skips the #if x86 and adds a 
comments about some of the deficiencies.
      The changes to strtod.c look OK.
                 Craig
--- gd_qnan.h.20150311	2015-03-11 10:27:17.160725969 -0400
+++ gd_qnan.h	2018-08-14 21:45:01.316310256 -0400
@@ -26,18 +26,22 @@
 #elif defined(__IEEE_LITTLE_ENDIAN)
 
 #if !defined(__mips)
-#define f_QNAN 0xffc00000
+#define f_QNAN 0x7fc00000
 #define d_QNAN0 0x0
-#define d_QNAN1 0xfff80000
+#define d_QNAN1 0x7ff80000
+/* NOTE (FIXME):  Newlib only can do long double in limited situations.  The
+ * ld_QNAN* and ldus_QNAN* defines are for Intel 80-bit.  Additional C function
+ * work (both here and in multiple *.c files) is needed to support standard
+ * 128-bit long double.  */
 #define ld_QNAN0 0x0
 #define ld_QNAN1 0xc0000000
-#define ld_QNAN2 0xffff
+#define ld_QNAN2 0x7fff
 #define ld_QNAN3 0x0
 #define ldus_QNAN0 0x0
 #define ldus_QNAN1 0x0
 #define ldus_QNAN2 0x0
 #define ldus_QNAN3 0xc000
-#define ldus_QNAN4 0xffff
+#define ldus_QNAN4 0x7fff
 #elif defined(__mips_nan2008)
 #define f_QNAN 0x7fc00000
 #define d_QNAN0 0x0
Masamichi Hosoda Aug. 15, 2018, 6:29 a.m. | #7
> On 08/14/2018 08:02 PM, Masamichi Hosoda wrote:

>>> On Wed, 15 Aug 2018, Masamichi Hosoda wrote:

>>>

>>>> On Linux with glibc, both strtod ("nan")

>>>> and strtod ("-nan") return positive NaN.

>>>>

>>>> So I've created the patch that behaves like glibc.

>>>> Both strtod ("nan") and strtod ("-nan") return positive NaN.

>>> I would suggest that you should not consider fixed bugs in glibc (bug

>>> 23007 in this case) to be appropriate to emulate in other libraries.

>> I've create the patch that behaves to preserve the sign bit.

>>

>> The result on Cygwin 64 bit (newlib, x86_64) with the patch:

>> ```

>> strtof ("nan", NULL) = nan

>> strtof ("-nan", NULL) = -nan

>> strtod ("nan", NULL) = nan

>> strtod ("-nan", NULL) = -nan

>> strtold ("nan", NULL) = nan

>> strtold ("-nan", NULL) = -nan

>> ```

>>

>> Thank you for your suggestion.

>      The f_QNAN value should be 0x7fc00000 regardless of byte

> ordering.  In addition, the d_QNAN* values should be 0x0 and

> 0x7FF80000, with only the index changing based on byte ordering.  So

> instead of them being inside of a x86/x86_64 define, they should just

> have their values corrected in the LITTLE_ENDIAN clause.

>      x86 does use a different coding for their 80-bit long double, so

> the ldus_QNAN* defines could belong within an x86 define.  On the

> other hand, the ldus_QNAN* defines only apply for Intel 80-bit, so in

> that respect don't need to be within a guard.

>      The ld_QNAN defines are not actually used anywhere.  If they

> were, however, Intel 80-bit would require different values than

> 128-bit.  However, the long double support in Newlib really only works

> when long double is the same size as double.  Some functions can work

> with Intel 80-bit, but almost none of them work with 128-bit.

>      Given the prior considerations, I suggest that only the values

> get fixed so that Cygwin works, but the #if x86 is not added.  For

> this to work on other platforms (128-bit long double), more work will

> be needed.  Again, sorry that I can't provide a git diff patch, but

> here's a suggested one with diff -pu.  It contains the value

> corrections from Masamichi, but skips the #if x86 and adds a comments

> about some of the deficiencies.

>      The changes to strtod.c look OK.


Thank you for your suggestion.
Here's patch v3.
From 4127a5473f6aaf546f09880826529c372c25643d Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Tue, 14 Aug 2018 22:29:34 +0900
Subject: [PATCH v3 1/2] Fix strtod ("nan") returns negative NaN

The definition of qNaN for x86_64 and i386 was wrong.
So strtod ("nan") and strtold ("nan") returned negative NaN
instead of positive NaN.

strtof ("nan") returns positive NaN so it does not have this issue.

This commit fixes definition of qNaN for x86_64 and i386.
So strtod ("nan") and strtold ("nan") return positive NaN.

Craig Howland <howland@LGSInnovations.com> suggested to skip
`#if defined (__x86_x64__) || defined (__i386__)`
block and to add a comments
for non-Intel (or 128-bit long double) platforms.
https://sourceware.org/ml/newlib/2018/msg00753.html
---
 newlib/libc/stdlib/gd_qnan.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/newlib/libc/stdlib/gd_qnan.h b/newlib/libc/stdlib/gd_qnan.h
index b775f82..0bfa7f3 100644
--- a/newlib/libc/stdlib/gd_qnan.h
+++ b/newlib/libc/stdlib/gd_qnan.h
@@ -26,18 +26,22 @@
 #elif defined(__IEEE_LITTLE_ENDIAN)
 
 #if !defined(__mips)
-#define f_QNAN 0xffc00000
+#define f_QNAN 0x7fc00000
 #define d_QNAN0 0x0
-#define d_QNAN1 0xfff80000
+#define d_QNAN1 0x7ff80000
+/* NOTE (FIXME):  Newlib only can do long double in limited situations.  The
+ * ld_QNAN* and ldus_QNAN* defines are for Intel 80-bit.  Additional C function
+ * work (both here and in multiple *.c files) is needed to support standard
+ * 128-bit long double.  */
 #define ld_QNAN0 0x0
 #define ld_QNAN1 0xc0000000
-#define ld_QNAN2 0xffff
+#define ld_QNAN2 0x7fff
 #define ld_QNAN3 0x0
 #define ldus_QNAN0 0x0
 #define ldus_QNAN1 0x0
 #define ldus_QNAN2 0x0
 #define ldus_QNAN3 0xc000
-#define ldus_QNAN4 0xffff
+#define ldus_QNAN4 0x7fff
 #elif defined(__mips_nan2008)
 #define f_QNAN 0x7fc00000
 #define d_QNAN0 0x0
-- 
2.17.0
From 6ae0ab8a7a27fc828ee66d8705f166f26c8f772c Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Wed, 15 Aug 2018 08:39:22 +0900
Subject: [PATCH v3 2/2] Fix strtof ("-nan") returns positive NaN

strtof ("-nan") returned positive NaN instead of negative NaN.
strtod ("-nan") and strtold ("-nan") return negative NaN.

Linux glibc has been fixed
that strto{f|d|ld} ("-nan") returns negative NaN.
https://sourceware.org/bugzilla/show_bug.cgi?id=23007

This commit makes strtof preserves the negative sign bit
when parsing "-nan" like glibc.
---
 newlib/libc/stdlib/strtod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 0cfa9e6..9c5ed56 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -1285,7 +1285,7 @@ strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)
 {
   double val = _strtod_l (_REENT, s00, se, loc);
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
@@ -1300,7 +1300,7 @@ strtof (const char *__restrict s00,
 {
   double val = _strtod_l (_REENT, s00, se, __get_current_locale ());
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
-- 
2.17.0
Corinna Vinschen Aug. 15, 2018, 9:07 a.m. | #8
On Aug 15 15:29, Masamichi Hosoda wrote:
> > On 08/14/2018 08:02 PM, Masamichi Hosoda wrote:

> >>> On Wed, 15 Aug 2018, Masamichi Hosoda wrote:

> >>>

> >>>> On Linux with glibc, both strtod ("nan")

> >>>> and strtod ("-nan") return positive NaN.

> >>>>

> >>>> So I've created the patch that behaves like glibc.

> >>>> Both strtod ("nan") and strtod ("-nan") return positive NaN.

> >>> I would suggest that you should not consider fixed bugs in glibc (bug

> >>> 23007 in this case) to be appropriate to emulate in other libraries.

> >> I've create the patch that behaves to preserve the sign bit.

> >>

> >> The result on Cygwin 64 bit (newlib, x86_64) with the patch:

> >> ```

> >> strtof ("nan", NULL) = nan

> >> strtof ("-nan", NULL) = -nan

> >> strtod ("nan", NULL) = nan

> >> strtod ("-nan", NULL) = -nan

> >> strtold ("nan", NULL) = nan

> >> strtold ("-nan", NULL) = -nan

> >> ```

> >>

> >> Thank you for your suggestion.

> >      The f_QNAN value should be 0x7fc00000 regardless of byte

> > ordering.  In addition, the d_QNAN* values should be 0x0 and

> > 0x7FF80000, with only the index changing based on byte ordering.  So

> > instead of them being inside of a x86/x86_64 define, they should just

> > have their values corrected in the LITTLE_ENDIAN clause.

> >      x86 does use a different coding for their 80-bit long double, so

> > the ldus_QNAN* defines could belong within an x86 define.  On the

> > other hand, the ldus_QNAN* defines only apply for Intel 80-bit, so in

> > that respect don't need to be within a guard.

> >      The ld_QNAN defines are not actually used anywhere.  If they

> > were, however, Intel 80-bit would require different values than

> > 128-bit.  However, the long double support in Newlib really only works

> > when long double is the same size as double.  Some functions can work

> > with Intel 80-bit, but almost none of them work with 128-bit.

> >      Given the prior considerations, I suggest that only the values

> > get fixed so that Cygwin works, but the #if x86 is not added.  For

> > this to work on other platforms (128-bit long double), more work will

> > be needed.  Again, sorry that I can't provide a git diff patch, but

> > here's a suggested one with diff -pu.  It contains the value

> > corrections from Masamichi, but skips the #if x86 and adds a comments

> > about some of the deficiencies.

> >      The changes to strtod.c look OK.

> 

> Thank you for your suggestion.

> Here's patch v3.


Looks good on Cygwin.  Everybody ok with this patch?


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Joseph Myers Aug. 15, 2018, 3:38 p.m. | #9
On Tue, 14 Aug 2018, Craig Howland wrote:

>      The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In


It would be better to use __builtin_nan ("") (and __builtin_nanf, 
__builtin_nanl for other types) rather than using an integer 
representation at all (of course that requires changes to other code to 
avoid requiring an integer representation there).

* Older MIPS processors have a reversed convention for what is a quiet NaN 
and what is a signaling NaN.  The present newlib code has __mips and 
__mips_nan2008 conditionals to handle that.  But that doesn't handle hppa, 
which has the same reversed convention, or sh, which also does (GCC 
doesn't know about sh doing this at present, resulting in a load of glibc 
test failures, but if you used __builtin_nan* then fewer places would need 
fixing for sh).

* Different processors have different preferences for what a "default" 
quiet NaN (e.g. one produced by an operation without NaN operands) should 
look like.  Some clear all the lower mantissa bits, some set them.  If you 
use the built-in functions then you generate NaNs following GCC's 
knowledge of such processor conventions.

-- 
Joseph S. Myers
joseph@codesourcery.com
Joseph Myers Aug. 15, 2018, 3:40 p.m. | #10
On Wed, 15 Aug 2018, Joseph Myers wrote:

> On Tue, 14 Aug 2018, Craig Howland wrote:

> 

> >      The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

> 

> It would be better to use __builtin_nan ("") (and __builtin_nanf, 

> __builtin_nanl for other types) rather than using an integer 

> representation at all (of course that requires changes to other code to 

> avoid requiring an integer representation there).


(This is not an objection to any of the present patch proposals, just an 
observation that a different approach would avoid a series of problems 
that result from trying to hardcode information about such choices of 
bit-patterns for NaNs.)

-- 
Joseph S. Myers
joseph@codesourcery.com
Craig Howland Aug. 15, 2018, 3:51 p.m. | #11
On 08/15/2018 11:40 AM, Joseph Myers wrote:
> On Wed, 15 Aug 2018, Joseph Myers wrote:

>

>> On Tue, 14 Aug 2018, Craig Howland wrote:

>>

>>>       The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

>> It would be better to use __builtin_nan ("") (and __builtin_nanf,

>> __builtin_nanl for other types) rather than using an integer

>> representation at all (of course that requires changes to other code to

>> avoid requiring an integer representation there).

I totally agree.  To add it to the record, in conjunction with this, the strtod 
implementation really should be upgraded to David Gay's more recent version, 
which is 128-bit friendly.  (I almost had this done some time ago, but didn't 
quite finish.)
> (This is not an objection to any of the present patch proposals, just an

> observation that a different approach would avoid a series of problems

> that result from trying to hardcode information about such choices of

> bit-patterns for NaNs.)

>

Also agreed.  If I had had time yesterday I might have tried it as I had briefly 
thought of it, but didn't think to get the idea out to the list, so I'm glad you 
did.
Corinna Vinschen Aug. 15, 2018, 4:05 p.m. | #12
On Aug 15 11:51, Craig Howland wrote:
> On 08/15/2018 11:40 AM, Joseph Myers wrote:

> > On Wed, 15 Aug 2018, Joseph Myers wrote:

> > 

> > > On Tue, 14 Aug 2018, Craig Howland wrote:

> > > 

> > > >       The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

> > > It would be better to use __builtin_nan ("") (and __builtin_nanf,

> > > __builtin_nanl for other types) rather than using an integer

> > > representation at all (of course that requires changes to other code to

> > > avoid requiring an integer representation there).

> I totally agree.  To add it to the record, in conjunction with this, the

> strtod implementation really should be upgraded to David Gay's more recent

> version, which is 128-bit friendly.  (I almost had this done some time ago,

> but didn't quite finish.)

> > (This is not an objection to any of the present patch proposals, just an

> > observation that a different approach would avoid a series of problems

> > that result from trying to hardcode information about such choices of

> > bit-patterns for NaNs.)

> > 

> Also agreed.  If I had had time yesterday I might have tried it as I had

> briefly thought of it, but didn't think to get the idea out to the list, so

> I'm glad you did.


Sounds like a nice followup patch...?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Paul Koning Aug. 15, 2018, 4:45 p.m. | #13
> On Aug 15, 2018, at 11:38 AM, Joseph Myers <joseph@codesourcery.com> wrote:

> 

> ...

> 

> * Different processors have different preferences for what a "default" 

> quiet NaN (e.g. one produced by an operation without NaN operands) should 

> look like.  Some clear all the lower mantissa bits, some set them.  If you 

> use the built-in functions then you generate NaNs following GCC's 

> knowledge of such processor conventions.


But does that matter?  It should be sufficient to generate a legal NaN when one is requested.  Given that there are don't care bits in the representation of NaN, it might look different from one generated by the hardware as the output of some operation, but a NaN is a Nan so long as it matches what the spec says -- right?

	paul
Joseph Myers Aug. 15, 2018, 4:51 p.m. | #14
On Wed, 15 Aug 2018, Paul Koning wrote:

> > * Different processors have different preferences for what a "default" 

> > quiet NaN (e.g. one produced by an operation without NaN operands) should 

> > look like.  Some clear all the lower mantissa bits, some set them.  If you 

> > use the built-in functions then you generate NaNs following GCC's 

> > knowledge of such processor conventions.

> 

> But does that matter?  It should be sufficient to generate a legal NaN 

> when one is requested.  Given that there are don't care bits in the 

> representation of NaN, it might look different from one generated by the 

> hardware as the output of some operation, but a NaN is a Nan so long as 

> it matches what the spec says -- right?


Well, it mattered enough for ARM to send a patch to the NaN bits set in 
ARM glibc fma 
<https://sourceware.org/ml/libc-alpha/2014-02/msg00740.html>.

-- 
Joseph S. Myers
joseph@codesourcery.com
Brian Inglis Aug. 15, 2018, 5:33 p.m. | #15
On 2018-08-15 10:51, Joseph Myers wrote:
> On Wed, 15 Aug 2018, Paul Koning wrote:

> 

>>> * Different processors have different preferences for what a "default" 

>>> quiet NaN (e.g. one produced by an operation without NaN operands) should 

>>> look like.  Some clear all the lower mantissa bits, some set them.  If you 

>>> use the built-in functions then you generate NaNs following GCC's 

>>> knowledge of such processor conventions.

>>

>> But does that matter?  It should be sufficient to generate a legal NaN 

>> when one is requested.  Given that there are don't care bits in the 

>> representation of NaN, it might look different from one generated by the 

>> hardware as the output of some operation, but a NaN is a Nan so long as 

>> it matches what the spec says -- right?


Signalling NaNs are useful in development and for mathematicians who want to
ensure rigorous correctness: most of us and most apps would prefer NaNs remain
quiet, and interchanged data with NaNs not to create a fuss: non-zero mantissa
bits which do not constitute a signalling NaN on any known platform [NaN with
all zero mantissa bits is Inf]. Some old K-C-S draft-based or low cost
implementations may cheap out on how many or which bit(s) to check.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
Masamichi Hosoda Aug. 16, 2018, 1:22 a.m. | #16
> On Aug 15 11:51, Craig Howland wrote:

>> On 08/15/2018 11:40 AM, Joseph Myers wrote:

>> > On Wed, 15 Aug 2018, Joseph Myers wrote:

>> > 

>> > > On Tue, 14 Aug 2018, Craig Howland wrote:

>> > > 

>> > > >       The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

>> > > It would be better to use __builtin_nan ("") (and __builtin_nanf,

>> > > __builtin_nanl for other types) rather than using an integer

>> > > representation at all (of course that requires changes to other code to

>> > > avoid requiring an integer representation there).

>> I totally agree.  To add it to the record, in conjunction with this, the

>> strtod implementation really should be upgraded to David Gay's more recent

>> version, which is 128-bit friendly.  (I almost had this done some time ago,

>> but didn't quite finish.)

>> > (This is not an objection to any of the present patch proposals, just an

>> > observation that a different approach would avoid a series of problems

>> > that result from trying to hardcode information about such choices of

>> > bit-patterns for NaNs.)

>> > 

>> Also agreed.  If I had had time yesterday I might have tried it as I had

>> briefly thought of it, but didn't think to get the idea out to the list, so

>> I'm glad you did.

> 

> Sounds like a nice followup patch...?


Here's patch v4.
It uses {nan|nanl} ("") instead of the integer representations of NaN.
It also removes the unused definitions of them.
From 7ed3011f720975c6e00968361e737a71fea3640c Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Thu, 16 Aug 2018 09:18:50 +0900
Subject: [PATCH v4 1/3] Fix strtod ("nan") and strtold ("nan") returns wrong
 negative NaN

The definition of qNaN for x86_64 and i386 was wrong.
strto{d|ld} ("nan") returned wrong negative NaN
instead of correct positive NaN
since it used the wrong definition.

On the other hand, strtof ("nan") returns correct positive NaN
since it uses nanf ("") instead of the wrong definition.

This commit makes strto{d|ld} ("nan") uses {nan|nanl} ("")
like strtof ("nan") using.
So strto{d|ld} ("nan") returns positive NaN.
---
 newlib/libc/stdlib/strtod.c  | 5 +----
 newlib/libc/stdlib/strtorx.c | 6 +-----
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 0cfa9e6..d70d2c2 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -444,10 +444,7 @@ _strtod_l (struct _reent *ptr, const char *__restrict s00, char **__restrict se,
 						}
 					else {
 #endif
-						dword0(rv) = NAN_WORD0;
-#ifndef _DOUBLE_IS_32BITS
-						dword1(rv) = NAN_WORD1;
-#endif /*!_DOUBLE_IS_32BITS*/
+						dval(rv) = nan ("");
 #ifndef No_Hex_NaN
 						}
 #endif
diff --git a/newlib/libc/stdlib/strtorx.c b/newlib/libc/stdlib/strtorx.c
index aeeb250..f923fdf 100644
--- a/newlib/libc/stdlib/strtorx.c
+++ b/newlib/libc/stdlib/strtorx.c
@@ -93,11 +93,7 @@ ULtox(__UShort *L, __ULong *bits, Long exp, int k)
 		break;
 
 	  case STRTOG_NaN:
-		L[0] = ldus_QNAN0;
-		L[1] = ldus_QNAN1;
-		L[2] = ldus_QNAN2;
-		L[3] = ldus_QNAN3;
-		L[4] = ldus_QNAN4;
+		*((long double*)L) = nanl ("");
 	  }
 	if (k & STRTOG_Neg)
 		L[_0] |= 0x8000;
-- 
2.17.0
From b94a348991122290aabb647a500ada21494b9d36 Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Thu, 16 Aug 2018 09:46:43 +0900
Subject: [PATCH v4 2/3] Remove unused NaN's integer representation definitions

By previous commit, strto{d|ld} ("nan")
does not use the definition of NaN.
There is no other function that uses the definitions.

This commit remove the definitions.
---
 newlib/libc/stdlib/gd_qnan.h      | 53 -------------------------------
 newlib/libc/stdlib/gdtoa-gethex.c |  1 -
 newlib/libc/stdlib/mprec.h        | 33 -------------------
 newlib/libc/stdlib/strtod.c       |  9 +++++-
 newlib/libc/stdlib/strtodg.c      |  1 -
 newlib/libc/stdlib/strtorx.c      |  4 ---
 6 files changed, 8 insertions(+), 93 deletions(-)
 delete mode 100644 newlib/libc/stdlib/gd_qnan.h

diff --git a/newlib/libc/stdlib/gd_qnan.h b/newlib/libc/stdlib/gd_qnan.h
deleted file mode 100644
index b775f82..0000000
--- a/newlib/libc/stdlib/gd_qnan.h
+++ /dev/null
@@ -1,53 +0,0 @@
-#ifdef __IEEE_BIG_ENDIAN
-
-#if !defined(__mips)
-#define f_QNAN 0x7fc00000
-#define d_QNAN0 0x7ff80000
-#define d_QNAN1 0x0
-#define ld_QNAN0 0x7ff80000
-#define ld_QNAN1 0x0
-#define ld_QNAN2 0x0
-#define ld_QNAN3 0x0
-#define ldus_QNAN0 0x7ff8
-#define ldus_QNAN1 0x0
-#define ldus_QNAN2 0x0
-#define ldus_QNAN3 0x0
-#define ldus_QNAN4 0x0
-#elif defined(__mips_nan2008)
-#define f_QNAN 0x7fc00000
-#define d_QNAN0 0x7ff80000
-#define d_QNAN1 0x0
-#else
-#define f_QNAN 0x7fbfffff
-#define d_QNAN0 0x7ff7ffff
-#define d_QNAN1 0xffffffff
-#endif
-
-#elif defined(__IEEE_LITTLE_ENDIAN)
-
-#if !defined(__mips)
-#define f_QNAN 0xffc00000
-#define d_QNAN0 0x0
-#define d_QNAN1 0xfff80000
-#define ld_QNAN0 0x0
-#define ld_QNAN1 0xc0000000
-#define ld_QNAN2 0xffff
-#define ld_QNAN3 0x0
-#define ldus_QNAN0 0x0
-#define ldus_QNAN1 0x0
-#define ldus_QNAN2 0x0
-#define ldus_QNAN3 0xc000
-#define ldus_QNAN4 0xffff
-#elif defined(__mips_nan2008)
-#define f_QNAN 0x7fc00000
-#define d_QNAN0 0x0
-#define d_QNAN1 0x7ff80000
-#else
-#define f_QNAN 0x7fbfffff
-#define d_QNAN0 0xffffffff
-#define d_QNAN1 0x7ff7ffff
-#endif
-
-#else
-#error IEEE endian not defined
-#endif
diff --git a/newlib/libc/stdlib/gdtoa-gethex.c b/newlib/libc/stdlib/gdtoa-gethex.c
index 939e0dd..d160015 100644
--- a/newlib/libc/stdlib/gdtoa-gethex.c
+++ b/newlib/libc/stdlib/gdtoa-gethex.c
@@ -35,7 +35,6 @@ THIS SOFTWARE.
 #include <locale.h>
 #include "mprec.h"
 #include "gdtoa.h"
-#include "gd_qnan.h"
 
 #if !defined(PREFER_SIZE_OVER_SPEED) && !defined(__OPTIMIZE_SIZE__) && !defined(_SMALL_HEXDIG)
 const unsigned char __hexdig[256]=
diff --git a/newlib/libc/stdlib/mprec.h b/newlib/libc/stdlib/mprec.h
index 7baec83..7e9a88b 100644
--- a/newlib/libc/stdlib/mprec.h
+++ b/newlib/libc/stdlib/mprec.h
@@ -265,39 +265,6 @@ typedef union { double d; __ULong i[2]; } U;
 #define INFNAN_CHECK
 #endif
 
-/*
- * NAN_WORD0 and NAN_WORD1 are only referenced in strtod.c.  Prior to
- * 20050115, they used to be hard-wired here (to 0x7ff80000 and 0,
- * respectively), but now are determined by compiling and running
- * qnan.c to generate gd_qnan.h, which specifies d_QNAN0 and d_QNAN1.
- * Formerly gdtoaimp.h recommended supplying suitable -DNAN_WORD0=...
- * and -DNAN_WORD1=...  values if necessary.  This should still work.
- * (On HP Series 700/800 machines, -DNAN_WORD0=0x7ff40000 works.)
- */
-#ifdef IEEE_Arith
-#ifdef IEEE_MC68k
-#define _0 0
-#define _1 1
-#ifndef NAN_WORD0
-#define NAN_WORD0 d_QNAN0
-#endif
-#ifndef NAN_WORD1
-#define NAN_WORD1 d_QNAN1
-#endif
-#else
-#define _0 1
-#define _1 0
-#ifndef NAN_WORD0
-#define NAN_WORD0 d_QNAN1
-#endif
-#ifndef NAN_WORD1
-#define NAN_WORD1 d_QNAN0
-#endif
-#endif
-#else
-#undef INFNAN_CHECK
-#endif
-
 #ifdef RND_PRODQUOT
 #define rounded_product(a,b) a = rnd_prod(a, b)
 #define rounded_quotient(a,b) a = rnd_quot(a, b)
diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index d70d2c2..3164e30 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -137,7 +137,6 @@ THIS SOFTWARE.
 #include <string.h>
 #include "mprec.h"
 #include "gdtoa.h"
-#include "gd_qnan.h"
 #include "../locale/setlocale.h"
 
 /* #ifndef NO_FENV_H */
@@ -172,6 +171,14 @@ static const double tinytens[] = { 1e-16, 1e-32,
 #define Rounding Flt_Rounds
 #endif
 
+#ifdef IEEE_MC68k
+#define _0 0
+#define _1 1
+#else
+#define _0 1
+#define _1 0
+#endif
+
 #ifdef Avoid_Underflow /*{*/
  static double
 sulp (U x,
diff --git a/newlib/libc/stdlib/strtodg.c b/newlib/libc/stdlib/strtodg.c
index 4ac1f8e..c8e581c 100644
--- a/newlib/libc/stdlib/strtodg.c
+++ b/newlib/libc/stdlib/strtodg.c
@@ -35,7 +35,6 @@ THIS SOFTWARE.
 #include <string.h>
 #include "mprec.h"
 #include "gdtoa.h"
-#include "gd_qnan.h"
 
 #include "locale.h"
 
diff --git a/newlib/libc/stdlib/strtorx.c b/newlib/libc/stdlib/strtorx.c
index f923fdf..a35dabe 100644
--- a/newlib/libc/stdlib/strtorx.c
+++ b/newlib/libc/stdlib/strtorx.c
@@ -35,13 +35,9 @@ THIS SOFTWARE.
 #include <string.h>
 #include "mprec.h"
 #include "gdtoa.h"
-#include "gd_qnan.h"
 
 #if defined (_HAVE_LONG_DOUBLE) && !defined (_LDBL_EQ_DBL)
 
-#undef _0
-#undef _1
-
 /* one or the other of IEEE_MC68k or IEEE_8087 should be #defined */
 
 #ifdef IEEE_MC68k
-- 
2.17.0
From 521bd9379f70d15c2f9848ed7ee3b7e4e3e6da72 Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>

Date: Wed, 15 Aug 2018 08:39:22 +0900
Subject: [PATCH v4 3/3] Fix strtof ("-nan") returns positive NaN

strtof ("-nan") returned positive NaN instead of negative NaN.
strtod ("-nan") and strtold ("-nan") return negative NaN.

Linux glibc has been fixed
that strto{f|d|ld} ("-nan") returns negative NaN.
https://sourceware.org/bugzilla/show_bug.cgi?id=23007

This commit makes strtof preserves the negative sign bit
when parsing "-nan" like glibc.
---
 newlib/libc/stdlib/strtod.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 3164e30..431d3ab 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -1289,7 +1289,7 @@ strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)
 {
   double val = _strtod_l (_REENT, s00, se, loc);
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
@@ -1304,7 +1304,7 @@ strtof (const char *__restrict s00,
 {
   double val = _strtod_l (_REENT, s00, se, __get_current_locale ());
   if (isnan (val))
-    return nanf (NULL);
+    return signbit (val) ? -nanf (NULL) : nanf (NULL);
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
-- 
2.17.0
Corinna Vinschen Aug. 16, 2018, 9:56 a.m. | #17
On Aug 16 10:22, Masamichi Hosoda wrote:
> > On Aug 15 11:51, Craig Howland wrote:

> >> On 08/15/2018 11:40 AM, Joseph Myers wrote:

> >> > On Wed, 15 Aug 2018, Joseph Myers wrote:

> >> > 

> >> > > On Tue, 14 Aug 2018, Craig Howland wrote:

> >> > > 

> >> > > >       The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

> >> > > It would be better to use __builtin_nan ("") (and __builtin_nanf,

> >> > > __builtin_nanl for other types) rather than using an integer

> >> > > representation at all (of course that requires changes to other code to

> >> > > avoid requiring an integer representation there).

> >> I totally agree.  To add it to the record, in conjunction with this, the

> >> strtod implementation really should be upgraded to David Gay's more recent

> >> version, which is 128-bit friendly.  (I almost had this done some time ago,

> >> but didn't quite finish.)

> >> > (This is not an objection to any of the present patch proposals, just an

> >> > observation that a different approach would avoid a series of problems

> >> > that result from trying to hardcode information about such choices of

> >> > bit-patterns for NaNs.)

> >> > 

> >> Also agreed.  If I had had time yesterday I might have tried it as I had

> >> briefly thought of it, but didn't think to get the idea out to the list, so

> >> I'm glad you did.

> > 

> > Sounds like a nice followup patch...?

> 

> Here's patch v4.

> It uses {nan|nanl} ("") instead of the integer representations of NaN.

> It also removes the unused definitions of them.


Still looks good on Cygwin.  I'll push this tomorrow, unless somebody
objects.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Corinna Vinschen Aug. 17, 2018, 9:38 a.m. | #18
On Aug 16 11:56, Corinna Vinschen wrote:
> On Aug 16 10:22, Masamichi Hosoda wrote:

> > > On Aug 15 11:51, Craig Howland wrote:

> > >> On 08/15/2018 11:40 AM, Joseph Myers wrote:

> > >> > On Wed, 15 Aug 2018, Joseph Myers wrote:

> > >> > 

> > >> > > On Tue, 14 Aug 2018, Craig Howland wrote:

> > >> > > 

> > >> > > >       The f_QNAN value should be 0x7fc00000 regardless of byte ordering.  In

> > >> > > It would be better to use __builtin_nan ("") (and __builtin_nanf,

> > >> > > __builtin_nanl for other types) rather than using an integer

> > >> > > representation at all (of course that requires changes to other code to

> > >> > > avoid requiring an integer representation there).

> > >> I totally agree.  To add it to the record, in conjunction with this, the

> > >> strtod implementation really should be upgraded to David Gay's more recent

> > >> version, which is 128-bit friendly.  (I almost had this done some time ago,

> > >> but didn't quite finish.)

> > >> > (This is not an objection to any of the present patch proposals, just an

> > >> > observation that a different approach would avoid a series of problems

> > >> > that result from trying to hardcode information about such choices of

> > >> > bit-patterns for NaNs.)

> > >> > 

> > >> Also agreed.  If I had had time yesterday I might have tried it as I had

> > >> briefly thought of it, but didn't think to get the idea out to the list, so

> > >> I'm glad you did.

> > > 

> > > Sounds like a nice followup patch...?

> > 

> > Here's patch v4.

> > It uses {nan|nanl} ("") instead of the integer representations of NaN.

> > It also removes the unused definitions of them.

> 

> Still looks good on Cygwin.  I'll push this tomorrow, unless somebody

> objects.


Pushed.


Thanks a lot,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Patch

From 7256702e5034b016b5114dd1a6c4c1a689a17816 Mon Sep 17 00:00:00 2001
From: Masamichi Hosoda <trueroad@trueroad.jp>
Date: Tue, 14 Aug 2018 23:12:49 +0900
Subject: [PATCH 2/2] Fix strtod ("-nan") returns negative NaN

On Linux,
glibc's strtod ("-nan") and strtold ("-nan") return positive NaN.

But, newlib's strtod ("-nan") returns negative NaN
because it inverted the sign with the presence of `-` character.
And, newlib's srtold ("-nan") returns negative NaN
because it set the sign bit with the presence of `-` character.

newlib's strtof ("-nan") returns positive NaN same as Linux glibc's.

This commit removes strtod's NaN sign inversion
and removes strtold's NaN sign bit setting.
So strtod ("-nan") and strtold ("-nan") return positive NaN
same as Linux glibc.
---
 newlib/libc/stdlib/strtod.c  | 1 +
 newlib/libc/stdlib/strtodg.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 0cfa9e6..3b9fd26 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -451,6 +451,7 @@  _strtod_l (struct _reent *ptr, const char *__restrict s00, char **__restrict se,
 #ifndef No_Hex_NaN
 						}
 #endif
+					sign = 0;
 					goto ret;
 					}
 			  }
diff --git a/newlib/libc/stdlib/strtodg.c b/newlib/libc/stdlib/strtodg.c
index 4ac1f8e..cc2842b 100644
--- a/newlib/libc/stdlib/strtodg.c
+++ b/newlib/libc/stdlib/strtodg.c
@@ -585,6 +585,7 @@  _strtodg_l (struct _reent *p, const char *s00, char **se, FPI *fpi, Long *exp,
 					if (*s == '(') /*)*/
 						irv = hexnan(&s, fpi, bits);
 #endif
+					sign = 0;
 					goto infnanexp;
 					}
 			  }
-- 
2.17.0