[1/4] Use nanf("") instead of nanf(NULL)

Message ID 20180827183231.25728-2-keithp@keithp.com
State Accepted
Commit 2c245028afa4883163c6ad7d04da9f7b7745b3b4
Headers show
Series
  • A selection of cleanups
Related show

Commit Message

Keith Packard Aug. 27, 2018, 6:32 p.m.
From: Keith Packard <keithp@keithp.com>


Newer GCC versions require a non-NULL argument to this function for
some reason.

Signed-off-by: Keith Packard <keithp@keithp.com>

---
 newlib/libc/stdio/nano-vfscanf_float.c | 2 +-
 newlib/libc/stdio/vfscanf.c            | 2 +-
 newlib/libc/stdio/vfwscanf.c           | 2 +-
 newlib/libc/stdlib/strtod.c            | 4 ++--
 newlib/libc/stdlib/wcstod.c            | 6 +++---
 5 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.18.0

Comments

Craig Howland Aug. 27, 2018, 9:21 p.m. | #1
On 08/27/2018 02:32 PM, keithp@keithp.com wrote:
> From: Keith Packard <keithp@keithp.com>

>

> Newer GCC versions require a non-NULL argument to this function for

> some reason.

>

> Signed-off-by: Keith Packard <keithp@keithp.com>

> ---

>   newlib/libc/stdio/nano-vfscanf_float.c | 2 +-

>   newlib/libc/stdio/vfscanf.c            | 2 +-

>   newlib/libc/stdio/vfwscanf.c           | 2 +-

>   newlib/libc/stdlib/strtod.c            | 4 ++--

>   newlib/libc/stdlib/wcstod.c            | 6 +++---

>   5 files changed, 8 insertions(+), 8 deletions(-)

>

> diff --git a/newlib/libc/stdio/nano-vfscanf_float.c b/newlib/libc/stdio/nano-vfscanf_float.c

> index a81fe7f70..5df9f227c 100644

> --- a/newlib/libc/stdio/nano-vfscanf_float.c

> +++ b/newlib/libc/stdio/nano-vfscanf_float.c

> @@ -330,7 +330,7 @@ fskip:

>   	{

>   	  flp = GET_ARG (N, *ap, float *);

>   	  if (isnan (fp))

> -	    *flp = nanf (NULL);

> +	    *flp = nanf ("");

>   	  else

>   	    *flp = fp;

>   	}

> ...

Do we want to allow this recent GCC change to force a source change, when the 
GCC change (added warning) is not a solid one?  The C standard does not require 
that the argument to nan() be non-null, although invoking general, indirect, 
rules (7.1.4) the behavior is perhaps undefined.  However, the Newlib nan() 
function ignores the argument, which means in Newlib's case the warning is 
spurious.

(The nan() definition is not totally clear, which is why I said "perhaps 
undefined" in the previous paragraph.  "If tagp does not point to an n-char 
sequence or an empty string, the call is equivalent to strtod("NAN", (char**) 
NULL)."  Does this statement include a NULL pointer--in which case the behavior 
is defined--or does it not--in which case the behavior is undefined by the 
standard, although it still is in Newlib's implementation.  A NULL pointer does 
not point to an n-char sequence or an empty string, thus it could be read this 
way, without needing to go to the general invalid argument (which include NULL 
pointer) in 7.1.4.  My personal preference in reading it is to understand that 
nan(NULL) is the same as strtod("NAN", (char**)NULL).)

Using GCC 6.2.1 for an ARM64 target, there is a real code generation difference 
between using nan("") and nan(NULL) (the latter of which gives a warning).  The 
former apparently changes the "nan("")" into "__builtin_nan("")", while the 
latter produces a function call to nan().  In other words, it appears this 
change is effectively replacing Newlib-internal nan() calls with __builtin_nan() 
calls.  I would guess that the same probably applies to any GCC version which 
emits the warning for nan(NULL) (which mine does).

I am only asking the question and pointing out a few facts related to the 
question, and don't mean to imply by asking that the change is not good, just 
that these things should be considered.  For my ARM64 case, the code is smaller 
and avoids a function call, so it is good from those points of view.

Craig
Joseph Myers Aug. 27, 2018, 10:10 p.m. | #2
On Mon, 27 Aug 2018, Craig Howland wrote:

> Do we want to allow this recent GCC change to force a source change, when the

> GCC change (added warning) is not a solid one?  The C standard does not

> require that the argument to nan() be non-null, although invoking general,


The argument must, as per DR#477 (resolution integrated in C17), be a 
string.

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2257.htm#dr_477

-- 
Joseph S. Myers
joseph@codesourcery.com
Craig Howland Aug. 27, 2018, 10:36 p.m. | #3
On 08/27/2018 06:10 PM, Joseph Myers wrote:
> On Mon, 27 Aug 2018, Craig Howland wrote:

>

>> Do we want to allow this recent GCC change to force a source change, when the

>> GCC change (added warning) is not a solid one?  The C standard does not

>> require that the argument to nan() be non-null, although invoking general,

> The argument must, as per DR#477 (resolution integrated in C17), be a

> string.

>

> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2257.htm#dr_477

>

Good that that is being taken care of.  While this does not change the fact that 
the change is not operationally needed in that the Newlib nan() ignores the 
argument, it does say changing for the sake the source being compliant (with the 
upcoming revision/clarification) should be done--even if generated code 
changes.  With that said, the edits look good.

Craig
Corinna Vinschen Aug. 29, 2018, 2:01 p.m. | #4
On Aug 27 11:32, keithp@keithp.com wrote:
> From: Keith Packard <keithp@keithp.com>

> 

> Newer GCC versions require a non-NULL argument to this function for

> some reason.

> 

> Signed-off-by: Keith Packard <keithp@keithp.com>

> ---

>  newlib/libc/stdio/nano-vfscanf_float.c | 2 +-

>  newlib/libc/stdio/vfscanf.c            | 2 +-

>  newlib/libc/stdio/vfwscanf.c           | 2 +-

>  newlib/libc/stdlib/strtod.c            | 4 ++--

>  newlib/libc/stdlib/wcstod.c            | 6 +++---

>  5 files changed, 8 insertions(+), 8 deletions(-)


Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Patch

diff --git a/newlib/libc/stdio/nano-vfscanf_float.c b/newlib/libc/stdio/nano-vfscanf_float.c
index a81fe7f70..5df9f227c 100644
--- a/newlib/libc/stdio/nano-vfscanf_float.c
+++ b/newlib/libc/stdio/nano-vfscanf_float.c
@@ -330,7 +330,7 @@  fskip:
 	{
 	  flp = GET_ARG (N, *ap, float *);
 	  if (isnan (fp))
-	    *flp = nanf (NULL);
+	    *flp = nanf ("");
 	  else
 	    *flp = fp;
 	}
diff --git a/newlib/libc/stdio/vfscanf.c b/newlib/libc/stdio/vfscanf.c
index 080dcf400..9c38eebf4 100644
--- a/newlib/libc/stdio/vfscanf.c
+++ b/newlib/libc/stdio/vfscanf.c
@@ -1886,7 +1886,7 @@  __SVFSCANF_R (struct _reent *rptr,
 		{
 		  flp = GET_ARG (N, ap, float *);
 		  if (isnan (res))
-		    *flp = nanf (NULL);
+		    *flp = nanf ("");
 		  else
 		    *flp = res;
 		}
diff --git a/newlib/libc/stdio/vfwscanf.c b/newlib/libc/stdio/vfwscanf.c
index c3470a15c..0464b0837 100644
--- a/newlib/libc/stdio/vfwscanf.c
+++ b/newlib/libc/stdio/vfwscanf.c
@@ -1636,7 +1636,7 @@  __SVFWSCANF_R (struct _reent *rptr,
 		{
 		  flp = GET_ARG (N, ap, float *);
 		  if (isnan (res))
-		    *flp = nanf (NULL);
+		    *flp = nanf ("");
 		  else
 		    *flp = res;
 		}
diff --git a/newlib/libc/stdlib/strtod.c b/newlib/libc/stdlib/strtod.c
index 431d3ab07..2a76b10ce 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 signbit (val) ? -nanf (NULL) : nanf (NULL);
+    return signbit (val) ? -nanf ("") : nanf ("");
   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 signbit (val) ? -nanf (NULL) : nanf (NULL);
+    return signbit (val) ? -nanf ("") : nanf ("");
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
diff --git a/newlib/libc/stdlib/wcstod.c b/newlib/libc/stdlib/wcstod.c
index 9e0d563ef..810c5b3fd 100644
--- a/newlib/libc/stdlib/wcstod.c
+++ b/newlib/libc/stdlib/wcstod.c
@@ -228,7 +228,7 @@  _wcstof_r (struct _reent *ptr,
 {
   double retval = _wcstod_l (ptr, nptr, endptr, __get_current_locale ());
   if (isnan (retval))
-    return nanf (NULL);
+    return nanf ("");
   return (float)retval;
 }
 
@@ -253,7 +253,7 @@  wcstof_l (const wchar_t *__restrict nptr, wchar_t **__restrict endptr,
 {
   double val = _wcstod_l (_REENT, nptr, endptr, loc);
   if (isnan (val))
-    return nanf (NULL);
+    return nanf ("");
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))
@@ -268,7 +268,7 @@  wcstof (const wchar_t *__restrict nptr,
 {
   double val = _wcstod_l (_REENT, nptr, endptr, __get_current_locale ());
   if (isnan (val))
-    return nanf (NULL);
+    return nanf ("");
   float retval = (float) val;
 #ifndef NO_ERRNO
   if (isinf (retval) && !isinf (val))