| Message ID | 5852C91D.2020109@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
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. >
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
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 >
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
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
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; }
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.