Set errno in strtof variants when conversion from double overflows

Message ID 5852C91D.2020109@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov Dec. 15, 2016, 4:47 p.m.
Hi all,

I've been investigating some libstdc++ execution failures on newlib bare-metal targets, in particular arm-none-eabi and aarch64-none-elf.
In particular:
21_strings/basic_string/numeric_conversions/char/stof.cc
21_strings/basic_string/numeric_conversions/wchar_t/stof.cc

These tests perform a conversion from string to a float and end up mapping down to the strtof/wcstof C functions.
They create a string from a large number that is representable in double precision but not in single precision and expect
strtof to return infinity and set errno to ERANGE so that libstdc++ can throw an exception.
But the implementation of strtof uses strtod internally to convert the string to a double and then cast the result to a float.
This works for getting the infinity result but doesn't set errno properly.
This patch adds a check after the conversion to see if the cast ended up generating an infinity from a non-infinity and sets errno
in that case.

With this patch the aforementioned tests pass on arm-none-eabi and aarch64-none-elf.

Is this ok?
If so, can someone please commit it on my behalf?

Thanks,
Kyrill

2016-12-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * libc/stdlib/strtod.c (strtof_l): Set errno to ERANGE when double to
     float conversion results in infinity.
     (strtof): Likewise.
     * libc/stdlib/wcstod.c (wcstof_l): Likewise.
     (wcstof): Likewise.

Comments

Jeff Johnston Dec. 15, 2016, 5:24 p.m. | #1
Patch has been applied.

Thanks,

-- Jeff J.

----- Original Message -----
> Hi all,

> 

> I've been investigating some libstdc++ execution failures on newlib

> bare-metal targets, in particular arm-none-eabi and aarch64-none-elf.

> In particular:

> 21_strings/basic_string/numeric_conversions/char/stof.cc

> 21_strings/basic_string/numeric_conversions/wchar_t/stof.cc

> 

> These tests perform a conversion from string to a float and end up mapping

> down to the strtof/wcstof C functions.

> They create a string from a large number that is representable in double

> precision but not in single precision and expect

> strtof to return infinity and set errno to ERANGE so that libstdc++ can throw

> an exception.

> But the implementation of strtof uses strtod internally to convert the string

> to a double and then cast the result to a float.

> This works for getting the infinity result but doesn't set errno properly.

> This patch adds a check after the conversion to see if the cast ended up

> generating an infinity from a non-infinity and sets errno

> in that case.

> 

> With this patch the aforementioned tests pass on arm-none-eabi and

> aarch64-none-elf.

> 

> Is this ok?

> If so, can someone please commit it on my behalf?

> 

> Thanks,

> Kyrill

> 

> 2016-12-15  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

> 

>      * libc/stdlib/strtod.c (strtof_l): Set errno to ERANGE when double to

>      float conversion results in infinity.

>      (strtof): Likewise.

>      * libc/stdlib/wcstod.c (wcstof_l): Likewise.

>      (wcstof): Likewise.

>
Corinna Vinschen Dec. 15, 2016, 8:03 p.m. | #2
On Dec 15 16:47, Kyrill Tkachov wrote:
> diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c

> index e908fcb..f82f507 100644

> --- a/newlib/libc/stdlib/strtod.c

> +++ b/newlib/libc/stdlib/strtod.c

> @@ -1293,9 +1293,14 @@ _DEFUN (strtod, (s00, se),

>  float

>  strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)

>  {

> -  double retval = _strtod_l (_REENT, s00, se, loc);

> -  if (isnan (retval))

> +  double val = _strtod_l (_REENT, s00, se, loc);

> +  if (isnan (val))

>      return nanf (NULL);

> +  float retval = (float) val;

> +#ifndef NO_ERRNO

> +  if (isinf (retval) && !isinf (val))

> +    _REENT->_errno = ERANGE;

> +#endif

>    return (float)retval;


            ^^^^^^^
Shouldn't this cast go, now that retval is already float?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat
Jeff Johnston Dec. 16, 2016, 4:30 p.m. | #3
It can go, but I would imagine it is doing no harm if the compiler has any
smarts at all.  I'll remove it.

-- Jeff J.

----- Original Message -----
> On Dec 15 16:47, Kyrill Tkachov wrote:

> > diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c

> > index e908fcb..f82f507 100644

> > --- a/newlib/libc/stdlib/strtod.c

> > +++ b/newlib/libc/stdlib/strtod.c

> > @@ -1293,9 +1293,14 @@ _DEFUN (strtod, (s00, se),

> >  float

> >  strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)

> >  {

> > -  double retval = _strtod_l (_REENT, s00, se, loc);

> > -  if (isnan (retval))

> > +  double val = _strtod_l (_REENT, s00, se, loc);

> > +  if (isnan (val))

> >      return nanf (NULL);

> > +  float retval = (float) val;

> > +#ifndef NO_ERRNO

> > +  if (isinf (retval) && !isinf (val))

> > +    _REENT->_errno = ERANGE;

> > +#endif

> >    return (float)retval;

> 

>             ^^^^^^^

> Shouldn't this cast go, now that retval is already float?

> 

> 

> Corinna

> 

> --

> Corinna Vinschen

> Cygwin Maintainer

> Red Hat

>
Corinna Vinschen Dec. 16, 2016, 6:27 p.m. | #4
On Dec 16 11:30, Jeff Johnston wrote:
> It can go, but I would imagine it is doing no harm if the compiler has any

> smarts at all.  I'll remove it.


Yeah, I didn't worry about the compiler, I was just thinking of code
cleanup.  Did the same to wcstod.c.


Thanks,
Corinna


> -- Jeff J.

> 

> ----- Original Message -----

> > On Dec 15 16:47, Kyrill Tkachov wrote:

> > > diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c

> > > index e908fcb..f82f507 100644

> > > --- a/newlib/libc/stdlib/strtod.c

> > > +++ b/newlib/libc/stdlib/strtod.c

> > > @@ -1293,9 +1293,14 @@ _DEFUN (strtod, (s00, se),

> > >  float

> > >  strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)

> > >  {

> > > -  double retval = _strtod_l (_REENT, s00, se, loc);

> > > -  if (isnan (retval))

> > > +  double val = _strtod_l (_REENT, s00, se, loc);

> > > +  if (isnan (val))

> > >      return nanf (NULL);

> > > +  float retval = (float) val;

> > > +#ifndef NO_ERRNO

> > > +  if (isinf (retval) && !isinf (val))

> > > +    _REENT->_errno = ERANGE;

> > > +#endif

> > >    return (float)retval;

> > 

> >             ^^^^^^^

> > Shouldn't this cast go, now that retval is already float?

> > 

> > 

> > Corinna
Kyrill Tkachov Dec. 19, 2016, 10:16 a.m. | #5
On 16/12/16 18:27, Corinna Vinschen wrote:
> On Dec 16 11:30, Jeff Johnston wrote:

>> It can go, but I would imagine it is doing no harm if the compiler has any

>> smarts at all.  I'll remove it.

> Yeah, I didn't worry about the compiler, I was just thinking of code

> cleanup.  Did the same to wcstod.c.


Thanks Jeff and Corinna.
I was testing a patch to delete these, but you beat me to it.

Kyrill

>

> Thanks,

> Corinna

>

>

>> -- Jeff J.

>>

>> ----- Original Message -----

>>> On Dec 15 16:47, Kyrill Tkachov wrote:

>>>> diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c

>>>> index e908fcb..f82f507 100644

>>>> --- a/newlib/libc/stdlib/strtod.c

>>>> +++ b/newlib/libc/stdlib/strtod.c

>>>> @@ -1293,9 +1293,14 @@ _DEFUN (strtod, (s00, se),

>>>>   float

>>>>   strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)

>>>>   {

>>>> -  double retval = _strtod_l (_REENT, s00, se, loc);

>>>> -  if (isnan (retval))

>>>> +  double val = _strtod_l (_REENT, s00, se, loc);

>>>> +  if (isnan (val))

>>>>       return nanf (NULL);

>>>> +  float retval = (float) val;

>>>> +#ifndef NO_ERRNO

>>>> +  if (isinf (retval) && !isinf (val))

>>>> +    _REENT->_errno = ERANGE;

>>>> +#endif

>>>>     return (float)retval;

>>>              ^^^^^^^

>>> Shouldn't this cast go, now that retval is already float?

>>>

>>>

>>> Corinna

Patch hide | download patch | download mbox

diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index e908fcb..f82f507 100644
--- a/newlib/libc/stdlib/strtod.c
+++ b/newlib/libc/stdlib/strtod.c
@@ -1293,9 +1293,14 @@  _DEFUN (strtod, (s00, se),
 float
 strtof_l (const char *__restrict s00, char **__restrict se, locale_t loc)
 {
-  double retval = _strtod_l (_REENT, s00, se, loc);
-  if (isnan (retval))
+  double val = _strtod_l (_REENT, s00, se, loc);
+  if (isnan (val))
     return nanf (NULL);
+  float retval = (float) val;
+#ifndef NO_ERRNO
+  if (isinf (retval) && !isinf (val))
+    _REENT->_errno = ERANGE;
+#endif
   return (float)retval;
 }
 
@@ -1304,9 +1309,14 @@  _DEFUN (strtof, (s00, se),
 	_CONST char *__restrict s00 _AND
 	char **__restrict se)
 {
-  double retval = _strtod_l (_REENT, s00, se, __get_current_locale ());
-  if (isnan (retval))
+  double val = _strtod_l (_REENT, s00, se, __get_current_locale ());
+  if (isnan (val))
     return nanf (NULL);
+  float retval = (float) val;
+#ifndef NO_ERRNO
+  if (isinf (retval) && !isinf (val))
+    _REENT->_errno = ERANGE;
+#endif
   return (float)retval;
 }
 
diff --git a/newlib/libc/stdlib/wcstod.c b/newlib/libc/stdlib/wcstod.c
index 43f7b4e..9b7948e 100644
--- a/newlib/libc/stdlib/wcstod.c
+++ b/newlib/libc/stdlib/wcstod.c
@@ -274,9 +274,14 @@  float
 wcstof_l (const wchar_t *__restrict nptr, wchar_t **__restrict endptr,
 	  locale_t loc)
 {
-  double retval = _wcstod_l (_REENT, nptr, endptr, loc);
-  if (isnan (retval))
+  double val = _wcstod_l (_REENT, nptr, endptr, loc);
+  if (isnan (val))
     return nanf (NULL);
+  float retval = (float) val;
+#ifndef NO_ERRNO
+  if (isinf (retval) && !isinf (val))
+    _REENT->_errno = ERANGE;
+#endif
   return (float)retval;
 }
 
@@ -285,9 +290,15 @@  _DEFUN (wcstof, (nptr, endptr),
 	_CONST wchar_t *__restrict nptr _AND
 	wchar_t **__restrict endptr)
 {
-  double retval = _wcstod_l (_REENT, nptr, endptr, __get_current_locale ());
-  if (isnan (retval))
+  double val = _wcstod_l (_REENT, nptr, endptr, __get_current_locale ());
+  if (isnan (val))
     return nanf (NULL);
+  float retval = (float) val;
+#ifndef NO_ERRNO
+  if (isinf (retval) && !isinf (val))
+    _REENT->_errno = ERANGE;
+#endif
+
   return (float)retval;
 }