macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

Hello.

I read the code of function `string-to-number', and traced functions or
macros recursively.

  traces of string-to-number:

    data.c: Fstring_to_number
      lisp.h: make_fixnum_or_float
        lisp.h: FIXNUM_OVERFLOW_P

  citation of FIXNUM_OVERFLOW_P (in Emacs 23.1):

    /* Value is non-zero if C integer I doesn't fit into a Lisp fixnum.  */

    #define FIXNUM_OVERFLOW_P(i) \
      ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
       || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)

I think FIXNUM_OVERFLOW_P is problematic.

The reason is that
in case `i' is 4294967296.0 (2^32), (EMACS_INT)(i) returns 0
with a executable program by some compiler
(for example, Microsoft 32-bit C/C++ Compiler).
As a result, FIXNUM_OVERFLOW_P(i) returns 0.

  In Microsoft 32-bit C/C++ Compiler:

    i					(int)(i)
    ===============================================
    2147483648.0	(2^31)		-2147483648
    2147483647.0	(2^31 - 1)	 2147483647

    4294967296.0	(2^32)		 0
    4294967295.0	(2^32 - 1)	-1

    8589934592.0	(2^33)		 0
    8589934591.0	(2^33 - 1)	 -1

  In gcc version 3.4.4 (cygming special):

    i					(int)(i)
    ===============================================
    2147483648.0	(2^31)		-2147483648
    2147483647.0	(2^31 - 1)	 2147483647

    4294967296.0	(2^32)		-2147483648
    4294967295.0	(2^32 - 1)	-2147483648

    8589934592.0	(2^33)		-2147483648
    8589934591.0	(2^33 - 1)	-2147483648

In conclusion, it needs casting to double, I think.

    #define FIXNUM_OVERFLOW_P(i) \
      ((double)(i) > (double)MOST_POSITIVE_FIXNUM \
       || (double)(i) < (double)MOST_NEGATIVE_FIXNUM)

What do you think about it ?

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> Date: Sat, 24 Oct 2009 02:51:47 +0900
> From: Toru TSUNEYOSHI <t_tuneyosi@hotmail.com>
> 
>     /* Value is non-zero if C integer I doesn't fit into a Lisp fixnum.  */
> 
>     #define FIXNUM_OVERFLOW_P(i) \
>       ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
>        || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)
> 
> I think FIXNUM_OVERFLOW_P is problematic.
> 
> The reason is that
> in case `i' is 4294967296.0 (2^32), (EMACS_INT)(i) returns 0
> with a executable program by some compiler

This is a known problem, but I'm not sure what would be the right
solution.  FIXNUM_OVERFLOW_P fails like that for any unsigned integer
that is large enough, because EMACS_INT is a signed type, so large
unsigned values become small negative values, and pass the test.

> In conclusion, it needs casting to double, I think.
> 
>     #define FIXNUM_OVERFLOW_P(i) \
>       ((double)(i) > (double)MOST_POSITIVE_FIXNUM \
>        || (double)(i) < (double)MOST_NEGATIVE_FIXNUM)
> 
> What do you think about it ?

This will not work on 64-bit platforms (where EMACS_INT is a 64-bit
type), because a double does not have 64 bits in the mantissa, and so
you will lose significant digits by that cast.

An alternative would be to have a separate test for unsigned values.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Andreas Schwab]

Toru TSUNEYOSHI <t_tuneyosi@hotmail.com> writes:

> I read the code of function `string-to-number', and traced functions or
> macros recursively.
>
>   traces of string-to-number:
>
>     data.c: Fstring_to_number
>       lisp.h: make_fixnum_or_float
>         lisp.h: FIXNUM_OVERFLOW_P
>
>   citation of FIXNUM_OVERFLOW_P (in Emacs 23.1):
>
>     /* Value is non-zero if C integer I doesn't fit into a Lisp fixnum.  */
>
>     #define FIXNUM_OVERFLOW_P(i) \
>       ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
>        || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)
>
> I think FIXNUM_OVERFLOW_P is problematic.

Thanks for the report.  The right fix is to remove the cast, so that the
compiler will promote the operands to the appropriate common type.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Stefan Monnier]

> I read the code of function `string-to-number', and traced functions or
> macros recursively.

>   traces of string-to-number:

>     data.c: Fstring_to_number
>       lisp.h: make_fixnum_or_float
>         lisp.h: FIXNUM_OVERFLOW_P

>   citation of FIXNUM_OVERFLOW_P (in Emacs 23.1):

>     /* Value is non-zero if C integer I doesn't fit into a Lisp fixnum.  */

>     #define FIXNUM_OVERFLOW_P(i) \
>       ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
>        || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)

> I think FIXNUM_OVERFLOW_P is problematic.

> The reason is that
> in case `i' is 4294967296.0 (2^32), (EMACS_INT)(i) returns 0
> with a executable program by some compiler
> (for example, Microsoft 32-bit C/C++ Compiler).
> As a result, FIXNUM_OVERFLOW_P(i) returns 0.

>   In Microsoft 32-bit C/C++ Compiler:

>     i					(int)(i)
>     ===============================================
>     2147483648.0	(2^31)		-2147483648
>     2147483647.0	(2^31 - 1)	 2147483647

>     4294967296.0	(2^32)		 0
>     4294967295.0	(2^32 - 1)	-1

>     8589934592.0	(2^33)		 0
>     8589934591.0	(2^33 - 1)	 -1

>   In gcc version 3.4.4 (cygming special):

>     i					(int)(i)
>     ===============================================
>     2147483648.0	(2^31)		-2147483648
>     2147483647.0	(2^31 - 1)	 2147483647

>     4294967296.0	(2^32)		-2147483648
>     4294967295.0	(2^32 - 1)	-2147483648

>     8589934592.0	(2^33)		-2147483648
>     8589934591.0	(2^33 - 1)	-2147483648

> In conclusion, it needs casting to double, I think.

>     #define FIXNUM_OVERFLOW_P(i) \
>       ((double)(i) > (double)MOST_POSITIVE_FIXNUM \
>        || (double)(i) < (double)MOST_NEGATIVE_FIXNUM)

> What do you think about it ?

I think the problem is that FIXNUM_OVERFLOW_P is written to handle an
integer argument, not a float argument.  So rather than patch it to use
floats, I'd rather write a new macro that works for floats (and only
casts the MOST_POSITIVE_FIXNUM side).


        Stefan

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Date: Fri, 23 Oct 2009 22:57:07 +0200
> Cc: emacs-devel@gnu.org
> 
> >     #define FIXNUM_OVERFLOW_P(i) \
> >       ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
> >        || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)
> >
> > I think FIXNUM_OVERFLOW_P is problematic.
> 
> Thanks for the report.  The right fix is to remove the cast, so that the
> compiler will promote the operands to the appropriate common type.

I think removing the cast will cause the compiler to whine on 64-bit
platforms about comparison being always right or wrong when the
argument is narrower than a 64-bit long.  IIRC, that's why the cast
was introduced in the first place.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Stefan Monnier]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andreas Schwab <schwab@linux-m68k.org>
>> Date: Fri, 23 Oct 2009 22:57:07 +0200
>> Cc: emacs-devel@gnu.org
>> 
>> >     #define FIXNUM_OVERFLOW_P(i) \
>> >       ((EMACS_INT)(i) > MOST_POSITIVE_FIXNUM \
>> >        || (EMACS_INT) (i) < MOST_NEGATIVE_FIXNUM)
>> >
>> > I think FIXNUM_OVERFLOW_P is problematic.
>> 
>> Thanks for the report.  The right fix is to remove the cast, so that the
>> compiler will promote the operands to the appropriate common type.

> I think removing the cast will cause the compiler to whine on 64-bit
> platforms about comparison being always right or wrong when the
> argument is narrower than a 64-bit long.  IIRC, that's why the cast
> was introduced in the first place.

Actually, when the cast was added in 2001, it came with the following
commit-log-comment:

  (FIXNUM_OVERFLOW_P): Cast I to EMACS_INT in comparisons
  in case I is of some unsigned type, in which case
  MOST_NEGATIVE_FIXNUM will be converted to unsigned, and the
  comparison becomes bogus.


-- Stefan

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

From: Eli Zaretskii <eliz@gnu.org>

>> In conclusion, it needs casting to double, I think.
>> 
>>     #define FIXNUM_OVERFLOW_P(i) \
>>       ((double)(i) > (double)MOST_POSITIVE_FIXNUM \
>>        || (double)(i) < (double)MOST_NEGATIVE_FIXNUM)
>> 
>> What do you think about it ?
> 
> This will not work on 64-bit platforms (where EMACS_INT is a 64-bit
> type), because a double does not have 64 bits in the mantissa, and so
> you will lose significant digits by that cast.
> 

(Regrettably, platform of machines I have is only 32-bit, and I don't
 know about 64-bit platforms.)

About string-to-number, it suppose that type of `i' to FIXNUM_OVERFLOW_P
is `double'.

  In string-to-number:

    {
      double v = 0;

      while (1)
	{
	  int digit = digit_to_number (*p++, b);
	  if (digit < 0)
	    break;
	  v = v * b + digit;
	}

      val = make_fixnum_or_float (sign * v); /* <= type of argument is `double' */
    }

So in 64-bit platforms, casting `double' to `int' causes lack of
precision already (because a double does not have 64 bits in the
mantissa), doesn't it?

(BTW, I wonder how application `bc' deals with overflow.)

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

[-- Attachment #1: Type: Text/Plain, Size: 342 bytes --]

> So in 64-bit platforms, casting `double' to `int' causes lack of
> precision already (because a double does not have 64 bits in the
> mantissa), doesn't it?

So I make a patch for keep the precision.
It tries to deal with the number in type `int' possibly, otherwise it
deals with the number in type `double'.

Would you like to check it ?

[-- Attachment #2: data.c.diff --]
[-- Type: Text/X-Patch, Size: 1064 bytes --]

--- data.c.original	2009-06-21 13:38:14.000000000 +0900
+++ data.c	2009-10-24 14:31:43.356164300 +0900
@@ -22,6 +22,7 @@
 #include <config.h>
 #include <signal.h>
 #include <stdio.h>
+#include <limits.h>
 #include "lisp.h"
 #include "puresize.h"
 #include "character.h"
@@ -2392,17 +2393,42 @@
     val = make_float (sign * atof (p));
   else
     {
-      double v = 0;
+      unsigned char *old_p = p;
+      int v = 0;
+      int overflow = 0;
 
-      while (1)
+      while (v >= 0)
 	{
 	  int digit = digit_to_number (*p++, b);
 	  if (digit < 0)
 	    break;
+	  if (v > INT_MAX / b)
+	    {
+	      overflow = 1;
+	      break;
+	    }
 	  v = v * b + digit;
 	}
 
-      val = make_fixnum_or_float (sign * v);
+      if (!overflow && v >= 0)
+	{
+	  val = make_fixnum_or_float (sign * v);
+	}
+      else
+	{
+	  double w = 0;
+	  p = old_p;
+
+	  while (1)
+	    {
+	      int digit = digit_to_number (*p++, b);
+	      if (digit < 0)
+		break;
+	      w = w * b + digit;
+	    }
+
+	  val = make_fixnum_or_float (sign * w);
+	}
     }
 
   return val;

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

[-- Attachment #1: Type: Text/Plain, Size: 380 bytes --]


>> So in 64-bit platforms, casting `double' to `int' causes lack of
>> precision already (because a double does not have 64 bits in the
>> mantissa), doesn't it?
> 
> So I make a patch for keep the precision.
> It tries to deal with the number in type `int' possibly, otherwise it
> deals with the number in type `double'.
> 
> Would you like to check it ?

I revised the patch.

[-- Attachment #2: data.c.diff --]
[-- Type: Text/X-Patch, Size: 1125 bytes --]

--- data.c.original	2009-06-21 13:38:14.000000000 +0900
+++ data.c	2009-10-24 16:32:17.805194400 +0900
@@ -22,6 +22,7 @@
 #include <config.h>
 #include <signal.h>
 #include <stdio.h>
+#include <limits.h>
 #include "lisp.h"
 #include "puresize.h"
 #include "character.h"
@@ -2392,17 +2393,46 @@
     val = make_float (sign * atof (p));
   else
     {
-      double v = 0;
+      unsigned char *old_p = p;
+      EMACS_INT v = 0;
+      int overflow = 0;
 
-      while (1)
+      while (v >= 0)
 	{
 	  int digit = digit_to_number (*p++, b);
 	  if (digit < 0)
 	    break;
+#ifdef _LP64
+	  if (v > LONG_MAX / b)
+#else
+	  if (v > INT_MAX / b)
+#endif
+	    {
+	      overflow = 1;
+	      break;
+	    }
 	  v = v * b + digit;
 	}
 
-      val = make_fixnum_or_float (sign * v);
+      if (!overflow && v >= 0)
+	{
+	  val = make_fixnum_or_float (sign * v);
+	}
+      else
+	{
+	  double w = 0;
+	  p = old_p;
+
+	  while (1)
+	    {
+	      int digit = digit_to_number (*p++, b);
+	      if (digit < 0)
+		break;
+	      w = w * b + digit;
+	    }
+
+	  val = make_fixnum_or_float (sign * w);
+	}
     }
 
   return val;

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> Date: Sat, 24 Oct 2009 10:50:33 +0900
> Cc: emacs-devel@gnu.org
> From: Toru TSUNEYOSHI <t_tuneyosi@hotmail.com>
> 
>       val = make_fixnum_or_float (sign * v); /* <= type of argument is `double' */
>     }
> 
> So in 64-bit platforms, casting `double' to `int' causes lack of
> precision already (because a double does not have 64 bits in the
> mantissa), doesn't it?

No.  The cast inside make_fixnum_or_float is to EMACS_INT, not to
`int'.  On 64-bit platforms, EMACS_INT is a 64-bit type, so casting a
`double' to EMACS_INT does not lose any precision.  Casting an
EMACS_INT to a `double' will cause loss of significant digits in the
EMACS_INT value.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 23 Oct 2009 21:05:05 -0400
> Cc: t_tuneyosi@hotmail.com, Andreas Schwab <schwab@linux-m68k.org>,
> 	emacs-devel@gnu.org
> 
> Actually, when the cast was added in 2001, it came with the following
> commit-log-comment:
> 
>   (FIXNUM_OVERFLOW_P): Cast I to EMACS_INT in comparisons
>   in case I is of some unsigned type, in which case
>   MOST_NEGATIVE_FIXNUM will be converted to unsigned, and the
>   comparison becomes bogus.

Right, sorry for my failing memory.

So I think the change made yesterday by Andreas should be reverted.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Fri, 23 Oct 2009 21:05:05 -0400
>> Cc: t_tuneyosi@hotmail.com, Andreas Schwab <schwab@linux-m68k.org>,
>> 	emacs-devel@gnu.org
>> 
>> Actually, when the cast was added in 2001, it came with the following
>> commit-log-comment:
>> 
>>   (FIXNUM_OVERFLOW_P): Cast I to EMACS_INT in comparisons
>>   in case I is of some unsigned type, in which case
>>   MOST_NEGATIVE_FIXNUM will be converted to unsigned, and the
>>   comparison becomes bogus.
>
> Right, sorry for my failing memory.
>
> So I think the change made yesterday by Andreas should be reverted.

The cast does not really fix the described problem anyway.  Converting
an unsigned value to a signed type when the value is out of range for
the signed type results in an implementation defined value.  That is
less bad than undefined behaviour, but still not portable.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Fri, 23 Oct 2009 21:05:05 -0400
>> Cc: t_tuneyosi@hotmail.com, Andreas Schwab <schwab@linux-m68k.org>,
>> 	emacs-devel@gnu.org
>> 
>> Actually, when the cast was added in 2001, it came with the following
>> commit-log-comment:
>> 
>>   (FIXNUM_OVERFLOW_P): Cast I to EMACS_INT in comparisons
>>   in case I is of some unsigned type, in which case
>>   MOST_NEGATIVE_FIXNUM will be converted to unsigned, and the
>>   comparison becomes bogus.
>
> Right, sorry for my failing memory.
>
> So I think the change made yesterday by Andreas should be reverted.

These should also work with unsigned types, but may generate extra
warnings:

#define FIXNUM_OVERFLOW_P(i) \
  (((i) > 0 && (i) > MOST_POSITIVE_FIXNUM) \
   || ((i) < 0 && (i) < MOST_NEGATIVE_FIXNUM))

or:

#define FIXNUM_OVERFLOW_P(i) \
  !((i) <= MOST_POSITIVE_FIXNUM \
    && (i) >= MOST_NEGATIVE_FIXNUM)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> Date: Sat, 24 Oct 2009 16:46:55 +0900
> Cc: emacs-devel@gnu.org
> From: Toru TSUNEYOSHI <t_tuneyosi@hotmail.com>
> 
> >> So in 64-bit platforms, casting `double' to `int' causes lack of
> >> precision already (because a double does not have 64 bits in the
> >> mantissa), doesn't it?
> > 
> > So I make a patch for keep the precision.
> > It tries to deal with the number in type `int' possibly, otherwise it
> > deals with the number in type `double'.
> > 
> > Would you like to check it ?
> 
> I revised the patch.

I think we should fix the make_fixnum_or_float macro, rather than work
around it in C code.

> +#ifdef _LP64
> +	  if (v > LONG_MAX / b)

This will probably not portable enough, e.g. on 64-bit Windows.  We
have MOST_POSITIVE_FIXNUM precisely for that reason.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  t_tuneyosi@hotmail.com,  emacs-devel@gnu.org
> Date: Sat, 24 Oct 2009 11:40:34 +0200
> 
> >>   (FIXNUM_OVERFLOW_P): Cast I to EMACS_INT in comparisons
> >>   in case I is of some unsigned type, in which case
> >>   MOST_NEGATIVE_FIXNUM will be converted to unsigned, and the
> >>   comparison becomes bogus.
> >
> > Right, sorry for my failing memory.
> >
> > So I think the change made yesterday by Andreas should be reverted.
> 
> The cast does not really fix the described problem anyway.  Converting
> an unsigned value to a signed type when the value is out of range for
> the signed type results in an implementation defined value.  That is
> less bad than undefined behaviour, but still not portable.

That's true, but I think we need to fix both problems.  In any case, I
think having a small unsigned value is more frequent than having a
very large unsigned value, which is probably the reason why the former
was discovered in 2001, while the latter only now.

Maybe the solution is to use a real function rather than a macro.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

From: Eli Zaretskii <eliz@gnu.org>

> No.  The cast inside make_fixnum_or_float is to EMACS_INT, not to
> `int'.  

Sorry, I misunderstood. :<

>	  On 64-bit platforms, EMACS_INT is a 64-bit type, so casting a
> `double' to EMACS_INT does not lose any precision.  Casting an
> EMACS_INT to a `double' will cause loss of significant digits in the
> EMACS_INT value.

OK.

BTW, how does Emacs on 64-bit platforms eval the following expressions?

    (string-to-number "1152921504606846975") ; 2^60 - 1
    =>                1.1529215046068467e+018 ; on 32-bit platforms

On 64-bit platforms, string-to-number should return the number as type
`EMACS_INT' (= `LONG'), I think.

Although, the code of string-to-number (Fstring_to_number) deals with
the number as type `double' (of variable `v').

So type `double' can't properly deal with (1st, 2nd, and 3rd) least
significant digits of the number, I think.

    #define make_fixnum_or_float(val) \
       (FIXNUM_OVERFLOW_P (val) \
        ? make_float (val) \
        : make_number ((EMACS_INT)(val)))

In this code, FIXNUM_OVERFLOW_P (val) will returns 0.
Then Emacs processes `make_number ((EMACS_INT)(val)))'.
On that time, `val' is casted from type `double' to `EMACS_INT'.
At last, can string-to-number return all digits of the number
1152921504606846975 properly?

I guessed it can't.
That is the reason why I made my former patch.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Eli Zaretskii]

> Date: Sat, 24 Oct 2009 20:59:47 +0900
> Cc: emacs-devel@gnu.org
> From: Toru TSUNEYOSHI <t_tuneyosi@hotmail.com>
> 
> BTW, how does Emacs on 64-bit platforms eval the following expressions?
> 
>     (string-to-number "1152921504606846975") ; 2^60 - 1
>     =>                1.1529215046068467e+018 ; on 32-bit platforms
> 
> On 64-bit platforms, string-to-number should return the number as type
> `EMACS_INT' (= `LONG'), I think.

Yes, it should.

> Although, the code of string-to-number (Fstring_to_number) deals with
> the number as type `double' (of variable `v').

Yes, it does.  And thus it loses least significant digits:

  (string-to-number "1152921504606846975") => 1152921504606846720

>     #define make_fixnum_or_float(val) \
>        (FIXNUM_OVERFLOW_P (val) \
>         ? make_float (val) \
>         : make_number ((EMACS_INT)(val)))
> 
> In this code, FIXNUM_OVERFLOW_P (val) will returns 0.
> Then Emacs processes `make_number ((EMACS_INT)(val)))'.
> On that time, `val' is casted from type `double' to `EMACS_INT'.
> At last, can string-to-number return all digits of the number
> 1152921504606846975 properly?
> 
> I guessed it can't.
> That is the reason why I made my former patch.

OK, but please make your patch compare against MOST_POSITIVE_FIXNUM,
instead of using INT_MAX or LONG_MAX.  Also, if you know that the
value does not overflow an EMACS_INT, you can simply call make_number,
instead of make_fixnum_or_float.

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Andreas Schwab]

Toru TSUNEYOSHI <t_tuneyosi@hotmail.com> writes:

> In this code, FIXNUM_OVERFLOW_P (val) will returns 0.
> Then Emacs processes `make_number ((EMACS_INT)(val)))'.
> On that time, `val' is casted from type `double' to `EMACS_INT'.
> At last, can string-to-number return all digits of the number
> 1152921504606846975 properly?
>
> I guessed it can't.
> That is the reason why I made my former patch.

Note that lread.c:read_integer has the same problem.  It is amazing how
many places try to convert a string of digits into a number, all failing
in one way or another.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Stefan Monnier]

> Note that lread.c:read_integer has the same problem.  It is amazing how
> many places try to convert a string of digits into a number, all failing
> in one way or another.

And all using different code.


        Stefan

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

[-- Attachment #1: Type: Text/Plain, Size: 1425 bytes --]

Thanks for your checking.

From: Eli Zaretskii <eliz@gnu.org>

> OK, but please make your patch compare against MOST_POSITIVE_FIXNUM,
> instead of using INT_MAX or LONG_MAX.  Also, if you know that the
> value does not overflow an EMACS_INT, you can simply call make_number,
> instead of make_fixnum_or_float.

I revised the patch as you wrote.
Would you like to check it?

;; supplement
;; check list (with results on 32-bit platforms)

(string-to-number "268435455")		; 2^28 - 1
=>		   268435455
(string-to-number "268435456")		; 2^28
=>		   268435456.0
(string-to-number "268435457")		; 2^28 + 1
=>		   268435457.0
(string-to-number "-268435456")		; - 2^28
=>		   -268435456
(string-to-number "-268435457")		; - 2^28 - 1
=>		   -268435457.0
(string-to-number "536870911")		; 2^29 - 1
=>		   536870911.0
(string-to-number "1073741822")		; 2^30 - 1
=>		   1073741822.0
(string-to-number "2147483647")		; 2^31 - 1
=>		   2147483647.0
(string-to-number "1152921504606846975") ; 2^60 - 1
=>		  1.1529215046068467e+018
(string-to-number "1152921504606846976") ; 2^60
=>		  1.1529215046068467e+018
(string-to-number "1152921504606846977") ; 2^60 + 1
=>		  1.1529215046068467e+018
(string-to-number "-1152921504606846976") ; - 2^60
=>		  -1.1529215046068467e+018
(string-to-number "-1152921504606846977") ; - 2^60 - 1
=>		  -1.1529215046068467e+018
(string-to-number "9223372036854775807") ; 2^63 - 1
=>		  9.223372036854778e+018

[-- Attachment #2: data.c.diff --]
[-- Type: Text/X-Patch, Size: 1055 bytes --]

--- data.c.orig	2009-06-21 13:38:14.000000000 +0900
+++ data.c	2009-10-25 03:35:54.414697700 +0900
@@ -2392,17 +2392,43 @@
     val = make_float (sign * atof (p));
   else
     {
-      double v = 0;
+      unsigned char *old_p = p;
+      EMACS_INT most_fixnum, chknum, v = 0;
+      int overflow = 0;		/* most_fixnum overflow */
 
-      while (1)
+      most_fixnum = sign > 0 ? MOST_POSITIVE_FIXNUM : MOST_NEGATIVE_FIXNUM * -1;
+      chknum = most_fixnum / b;
+
+      while (v >= 0)
 	{
 	  int digit = digit_to_number (*p++, b);
 	  if (digit < 0)
 	    break;
+	  if (chknum < v / b)
+	    {
+	      overflow = 1;
+	      break;
+	    }
 	  v = v * b + digit;
 	}
 
-      val = make_fixnum_or_float (sign * v);
+      if (!overflow && v <= most_fixnum && v >= 0)
+	val = make_number (sign * v);
+      else
+	{
+	  double w = 0;
+	  p = old_p;
+
+	  while (1)
+	    {
+	      int digit = digit_to_number (*p++, b);
+	      if (digit < 0)
+		break;
+	      w = w * b + digit;
+	    }
+
+	  val = make_float (sign * w);
+	}
     }
 
   return val;

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

From: Stefan Monnier <monnier@IRO.UMontreal.CA>

> I think the problem is that FIXNUM_OVERFLOW_P is written to handle an
> integer argument, not a float argument.  So rather than patch it to use
> floats, I'd rather write a new macro that works for floats (and only
> casts the MOST_POSITIVE_FIXNUM side).
> 
> 
>         Stefan

I made a new macro.
Probably, it doesn't depend on variable type, I think.
Would you like to check it?

#define FIXNUM_OVERFLOW_P(i)		 \
  ((i) > 0				 \
   ? (i) != MOST_POSITIVE_FIXNUM	 \
     && (i) / MOST_POSITIVE_FIXNUM >= 1	 \
   : (i) != MOST_NEGATIVE_FIXNUM	 \
     && (i) / MOST_NEGATIVE_FIXNUM >= 1)

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

I revised the macro.
(My former macro may be problematic on 64-bit platforms.)
Probably, the following is no problems in 64-bit platforms, I think.

/* citation from mktime.c ---> */

#ifndef CHAR_BIT
# define CHAR_BIT 8
#endif

/* The extra casts work around common compiler bugs.  */
#define TYPE_SIGNED(t) (! ((t) 0 < (t) -1))
/* The outer cast is needed to work around a bug in Cray C 5.0.3.0.
   It is necessary at least when t == time_t.  */
#define TYPE_MINIMUM(t) ((t) (TYPE_SIGNED (t) \
			      ? ~ (t) 0 << (sizeof (t) * CHAR_BIT - 1) : (t) 0))
#define TYPE_MAXIMUM(t) ((t) (~ (t) 0 - TYPE_MINIMUM (t)))

/* <--- citation from mktime.c */

#ifndef EMACS_INT_MIN
# define EMACS_INT_MIN TYPE_MINIMUM (EMACS_INT)
#endif
#ifndef EMACS_INT_MAX
# define EMACS_INT_MAX TYPE_MAXIMUM (EMACS_INT)
#endif

#define FIXNUM_OVERFLOW_P(i)					  \
  ((i) < 0							  \
   ? (i) < EMACS_INT_MIN || (EMACS_INT)(i) < MOST_NEGATIVE_FIXNUM \
   : (i) > EMACS_INT_MAX || (EMACS_INT)(i) > MOST_POSITIVE_FIXNUM)

Re: macro FIXNUM_OVERFLOW_P in lisp.h is valid ?

[Toru TSUNEYOSHI]

Thanks for your checking.

From: Andreas Schwab <schwab@linux-m68k.org>

> Note that lread.c:read_integer has the same problem.  It is amazing how
> many places try to convert a string of digits into a number, all failing
> in one way or another.

I didn't know lread.c:read_integer well. (So I read the code a little.)
But I think this function is allowed to overflow if we interpret the
word `integer' (not `number') literally.