Buffer size limitation in insdel.c

Buffer size limitation in insdel.c

[Eli Zaretskii]

We have this in insdel.c:make_gap_larger:

  { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
    if (total_size < 0
	/* Don't allow a buffer size that won't fit in a Lisp integer.  */
	|| total_size != XINT (make_number (total_size))
	/* Don't allow a buffer size that won't fit in an int
	   even if it will fit in a Lisp integer.
	   That won't work because so many places still use `int'.  */
	|| total_size != (EMACS_INT) (int) total_size)
      error ("Buffer exceeds maximum size");

"bzr annotate" says this was committed a year ago by Stefan.

Any objections to removing this limitation on 64-bit machines?

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We have this in insdel.c:make_gap_larger:
>
>   { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
>     if (total_size < 0
> 	/* Don't allow a buffer size that won't fit in a Lisp integer.  */
> 	|| total_size != XINT (make_number (total_size))
> 	/* Don't allow a buffer size that won't fit in an int
> 	   even if it will fit in a Lisp integer.
> 	   That won't work because so many places still use `int'.  */
> 	|| total_size != (EMACS_INT) (int) total_size)
>       error ("Buffer exceeds maximum size");
>
> "bzr annotate" says this was committed a year ago by Stefan.
>
> Any objections to removing this limitation on 64-bit machines?

If you saw my experiment with -Wconversion, we have 5400 potential type
conversion errors, where a lot (a *lot*) of them involved EMACS_INT/int
confusion, and a lot of those were buffer-related.

So until that's fixed, I think it's a valid limitation.  Emacs is
probably just going to segfault if you load a buffer that's bigger than
(er) 2GB.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 22 Sep 2010 14:12:02 +0200
> 
> >   { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
> >     if (total_size < 0
> > 	/* Don't allow a buffer size that won't fit in a Lisp integer.  */
> > 	|| total_size != XINT (make_number (total_size))
> > 	/* Don't allow a buffer size that won't fit in an int
> > 	   even if it will fit in a Lisp integer.
> > 	   That won't work because so many places still use `int'.  */
> > 	|| total_size != (EMACS_INT) (int) total_size)
> >       error ("Buffer exceeds maximum size");
> >
> > "bzr annotate" says this was committed a year ago by Stefan.
> >
> > Any objections to removing this limitation on 64-bit machines?
> 
> If you saw my experiment with -Wconversion, we have 5400 potential type
> conversion errors, where a lot (a *lot*) of them involved EMACS_INT/int
> confusion, and a lot of those were buffer-related.
> 
> So until that's fixed, I think it's a valid limitation.  Emacs is
> probably just going to segfault if you load a buffer that's bigger than
> (er) 2GB.

Then why do other places that bitch about buffer size being exceeded
don't do the same?

Re: Buffer size limitation in insdel.c

[David Kastrup]

Eli Zaretskii <eliz@gnu.org> writes:

> We have this in insdel.c:make_gap_larger:
>
>   { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
>     if (total_size < 0
> 	/* Don't allow a buffer size that won't fit in a Lisp integer.  */
> 	|| total_size != XINT (make_number (total_size))
> 	/* Don't allow a buffer size that won't fit in an int
> 	   even if it will fit in a Lisp integer.
> 	   That won't work because so many places still use `int'.  */
> 	|| total_size != (EMACS_INT) (int) total_size)
>       error ("Buffer exceeds maximum size");
>
> "bzr annotate" says this was committed a year ago by Stefan.
>
> Any objections to removing this limitation on 64-bit machines?

I should think that this would call for a code review of Emacs' C
modules first.

-- 
David Kastrup

Re: Buffer size limitation in insdel.c

[Stefan Monnier]

> "bzr annotate" says this was committed a year ago by Stefan.
> Any objections to removing this limitation on 64-bit machines?

Fine by me, but only if you've confirmed that Emacs is able to display
a 3GB file and navigate in it.  Last I tried it failed miserably.


        Stefan

Re: Buffer size limitation in insdel.c

[Richard Stallman]

      { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
	if (total_size < 0
	    /* Don't allow a buffer size that won't fit in a Lisp integer.  */
	    || total_size != XINT (make_number (total_size))
	    /* Don't allow a buffer size that won't fit in an int
	       even if it will fit in a Lisp integer.
	       That won't work because so many places still use `int'.  */
	    || total_size != (EMACS_INT) (int) total_size)
	  error ("Buffer exceeds maximum size");

    "bzr annotate" says this was committed a year ago by Stefan.

    Any objections to removing this limitation on 64-bit machines?

Is there a reason to believe it is not necessary any more?

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> CC: emacs-devel@gnu.org
> Date: Wed, 22 Sep 2010 20:58:59 -0400
> 
>       { EMACS_INT total_size = Z_BYTE - BEG_BYTE + GAP_SIZE + nbytes_added;
> 	if (total_size < 0
> 	    /* Don't allow a buffer size that won't fit in a Lisp integer.  */
> 	    || total_size != XINT (make_number (total_size))
> 	    /* Don't allow a buffer size that won't fit in an int
> 	       even if it will fit in a Lisp integer.
> 	       That won't work because so many places still use `int'.  */
> 	    || total_size != (EMACS_INT) (int) total_size)
> 	  error ("Buffer exceeds maximum size");
> 
>     "bzr annotate" says this was committed a year ago by Stefan.
> 
>     Any objections to removing this limitation on 64-bit machines?
> 
> Is there a reason to believe it is not necessary any more?

It hides bugs, if nothing else.  But I understand people prefer to
keep it for now, for fear that Emacs will break completely on 64-bit
hosts.  The problem is that no one seems to be working on fixing those
bugs.

Re: Buffer size limitation in insdel.c

[Leo]

On 2010-09-23 08:55 +0100, Eli Zaretskii wrote:
> It hides bugs, if nothing else. But I understand people prefer to keep
> it for now, for fear that Emacs will break completely on 64-bit hosts.
> The problem is that no one seems to be working on fixing those bugs.

Maybe do it in a separate branch and merge it later on?

Leo

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Leo <sdl.web@gmail.com>
> Date: Thu, 23 Sep 2010 10:23:00 +0100
> 
> On 2010-09-23 08:55 +0100, Eli Zaretskii wrote:
> > It hides bugs, if nothing else. But I understand people prefer to keep
> > it for now, for fear that Emacs will break completely on 64-bit hosts.
> > The problem is that no one seems to be working on fixing those bugs.
> 
> Maybe do it in a separate branch and merge it later on?

I think it's better to do this incrementally in the trunk, because
this means more extensive testing.  Each incremental set of changes
(see two such changes I committed yesterday) should be safe, so
there's no need to use a separate branch.  The final test is, as
Stefan says, being able to visit and navigate a file larger than 2GB,
at which point the limitation should be lifted.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It hides bugs, if nothing else.  But I understand people prefer to
> keep it for now, for fear that Emacs will break completely on 64-bit
> hosts.  The problem is that no one seems to be working on fixing those
> bugs.

There were no comments on my -Wconversion post, so I thought there was
no interest in making the int/EMACS_INT overflows go away.

The bugs are trivial to find with -Wconversion, and most of them are
easy enough to fix.  But some may be more subtle?

A methodology to fix this up with be for us to compile with
-Wconversion, pick a file, any file, to work on, fix all the
-Wconversion warnings in that file, and then see if Emacs still works.

Then check in and move on to the next file.  :-)

I'm up for taking a few files, but I'm not up for doing all 5400
warnings.  (Although there are probably fewer things to be fixed.  int a
= PV; a = Z; will probably give two warnings?  So one fix will kill one
or more warnings.)

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 12:59:56 +0200
> 
> There were no comments on my -Wconversion post, so I thought there was
> no interest in making the int/EMACS_INT overflows go away.

You have a lot to learn about the corporate culture here ;-)  Lack of
response does not necessarily mean lack of interest.  It just means no
one volunteers yet.

> The bugs are trivial to find with -Wconversion, and most of them are
> easy enough to fix.  But some may be more subtle?

Some are, indeed.  But those are few and far in between.

> A methodology to fix this up with be for us to compile with
> -Wconversion, pick a file, any file, to work on, fix all the
> -Wconversion warnings in that file, and then see if Emacs still works.
> 
> Then check in and move on to the next file.  :-)
> 
> I'm up for taking a few files, but I'm not up for doing all 5400
> warnings.

I already started, feel free to join, and thanks.

For now, I simply go through the sources by hand, it's easy enough to
spot the obvious problems.  In a nutshell, any variable declared as
`int' is a suspect, at least in some source files that are frequent
offenders.  When this is out of my way, I will use -Wconversion.  That
sounds easier than going through 5000 warnings; YMMV.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> You have a lot to learn about the corporate culture here ;-)  Lack of
> response does not necessarily mean lack of interest.  It just means no
> one volunteers yet.

Right.  :-)

> I already started, feel free to join, and thanks.

Ok; I'll take a whack at fixing up a file tonight and see how things
work out.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

A few general questions:

dispnew.c:783: warning: conversion to 'int' from 'Lisp_Object' may alter its value

      matrix->window_left_col = WINDOW_LEFT_EDGE_COL (w);

What's the right fix here?  make window_left_col EMACS_INT, and slap an
XINT around the WINDOW_LEFT_EDGE_COL?

Are these interesting at all?  That is, conversions that may
theoretically change signs, but it's really unlikely?

dispnew.c:2178: warning: conversion to 'long unsigned int' from 'short int' may change the sign of the result

      size_t nbytes = from->used[TEXT_AREA] * sizeof (struct glyph);

Perhaps dispnew.c wasn't an optimal place to start poking around.  :-)      

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Andreas Schwab]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> dispnew.c:783: warning: conversion to 'int' from 'Lisp_Object' may alter its value

This is an obvious bug.  A Lisp_Object value must never be treated as an
integer, and this will fail to compile with --enable-use-lisp-union-type
(which is a good idea to enable during development).

> dispnew.c:2178: warning: conversion to 'long unsigned int' from 'short int' may change the sign of the result
>
>       size_t nbytes = from->used[TEXT_AREA] * sizeof (struct glyph);

This is only a problem if from->used[TEXT_AREA] can be negative, which
it can't.  Might be worth to change to unsigned short.

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: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 14:47:23 +0200
> 
> A few general questions:
> 
> dispnew.c:783: warning: conversion to 'int' from 'Lisp_Object' may alter its value
> 
>       matrix->window_left_col = WINDOW_LEFT_EDGE_COL (w);
> 
> What's the right fix here?  make window_left_col EMACS_INT, and slap an
> XINT around the WINDOW_LEFT_EDGE_COL?

I don't see this warning with "gcc (GCC) 4.2.4 (Ubuntu 4.2.4-1ubuntu4)".
And I don't see how you could possibly get it, since
window_left_edge_col is an int, whereas WINDOW_LEFT_EDGE_COL is
defined like this:

    #define WINDOW_LEFT_EDGE_COL(W) \
      (XFASTINT ((W)->left_col))

So it already extracts the integer from a Lisp_Object.  What am I
missing?


> dispnew.c:2178: warning: conversion to 'long unsigned int' from 'short int' may change the sign of the result
> 
>       size_t nbytes = from->used[TEXT_AREA] * sizeof (struct glyph);

Something like

  size_t nbytes;
  if (from->used[TEXT_AREA] > 0)
    nbytes = from->used[TEXT_AREA] * sizeof (struct glyph);
  else
    nbytes = 0;

Or just leave it alone, it obviously isn't in the way of using large
buffers on 64-bit hosts.

And btw, I don't see that warning, either.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> dispnew.c:783: warning: conversion to 'int' from 'Lisp_Object' may alter its value
>> 
>>       matrix->window_left_col = WINDOW_LEFT_EDGE_COL (w);
>> 
>> What's the right fix here?  make window_left_col EMACS_INT, and slap an
>> XINT around the WINDOW_LEFT_EDGE_COL?
>
> I don't see this warning with "gcc (GCC) 4.2.4 (Ubuntu 4.2.4-1ubuntu4)".

This is with "gcc (Debian 4.4.4-7) 4.4.4".  When I was Googling for
-Wconversion, I found somebody saying that it's a switch that has
existed for a long time, but it was recently given a total makeover, and
was made to be more useful.

> And I don't see how you could possibly get it, since
> window_left_edge_col is an int, whereas WINDOW_LEFT_EDGE_COL is
> defined like this:
>
>     #define WINDOW_LEFT_EDGE_COL(W) \
>       (XFASTINT ((W)->left_col))
>
> So it already extracts the integer from a Lisp_Object.  What am I
> missing?

Hm.  What version of XFASTINT is being used by default?  If it's this
one, I can perhaps understand the warning.  I think.  

#define XFASTINT(a) ((a) + 0)

So gcc 4.4.4 doesn't understand that this converts from Lisp_Object a to
EMACS_INT a?  Which is what I'm assuming it's doing, although I'm not
sure whether the C standard actually allows doing it this way?

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 15:37:35 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > And I don't see how you could possibly get it, since
> > window_left_edge_col is an int, whereas WINDOW_LEFT_EDGE_COL is
> > defined like this:
> >
> >     #define WINDOW_LEFT_EDGE_COL(W) \
> >       (XFASTINT ((W)->left_col))
> >
> > So it already extracts the integer from a Lisp_Object.  What am I
> > missing?
> 
> Hm.  What version of XFASTINT is being used by default?  If it's this
> one, I can perhaps understand the warning.  I think.  
> 
> #define XFASTINT(a) ((a) + 0)

Use "gcc -E" to see.  In my case, the result is this:

   matrix->window_left_col = (((((long) ((w)->left_col)) >> (3 - 1))));

which looks correct, unless I'm totally confused.

So I think the definition used, at least in my case, is this:

  #ifndef XFASTINT
  # define XFASTINT(a) (XINT (a))

because XINT has this definition:

  # define XINT(a) (((EMACS_INT) (a)) >> (GCTYPEBITS - 1))

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Use "gcc -E" to see.

Ah, thanks.  That helps a lot in figuring out what the code actually is.

> In my case, the result is this:
>
>    matrix->window_left_col = (((((long) ((w)->left_col)) >> (3 - 1))));
>
> which looks correct, unless I'm totally confused.

I get the same in my gcc:

      matrix->window_left_col = (((((long) ((w)->left_col)) >> (3 - 1))));

So the warning is incorrect -- it's no longer a Lisp_Object at that
point.  But it's correct in that it's a potentially problematic
conversion.  (But probably isn't in actuality, since it's unlikely
w->left_col really is a more than 31 bits.)

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> So the warning is incorrect -- it's no longer a Lisp_Object at that
> point.  But it's correct in that it's a potentially problematic
> conversion. 

Yup.  It's just naming the original type before the cast, apparently.
Like here:

  dispnew.c:520: warning: conversion to 'int' from 'Lisp_Object' may alter its value

  int width = XFASTINT (w->total_cols);

While stuff that's already EMACS_INT get the more correct warning:  

  dispnew.c:4708: warning: conversion to 'int' from 'long int' may alter its value

    int preempt_count = baud_rate / 2400 + 1;

I think you're right that it might be more productive to just read the
code than using -Wconversion.  That means that we'll never be able to
get the compiler to warn us the next time somebody writes int i = Z;,
which is a shame.  Compilers are really good at finding type errors.
:-)     
    
-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 16:21:58 +0200
> 
>       matrix->window_left_col = (((((long) ((w)->left_col)) >> (3 - 1))));
> 
> So the warning is incorrect -- it's no longer a Lisp_Object at that
> point.  But it's correct in that it's a potentially problematic
> conversion.  (But probably isn't in actuality, since it's unlikely
> w->left_col really is a more than 31 bits.)

An explicit cast to int should shut it up (but you never know, with
GCC 4.x...).

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Ok, I think I've found a way to zoom in on this specific problem pretty
easily.  If we only concern ourselves with conversion from EMACS_INT to
int, then the following compile command gives us that:

make -k 2>&1 | egrep 'conversion.*int.*(Lisp_Ob|long)'

Here's an example from fns.c, with 131 hits for this error, and
eyeballing them, they mostly seem like valid errors.

make -k 2>&1 | egrep 'conversion.*int.*(Lisp_Ob|long)'
fns.c:110: warning: conversion to 'long unsigned int' from 'Lisp_Object' may change the sign of the result
fns.c:112: warning: conversion to 'long unsigned int' from 'long int' may change the sign of the result
fns.c:112: warning: conversion to 'long int' from 'long unsigned int' may change the sign of the result
fns.c:260: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:261: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:263: warning: conversion to 'int' from 'long int' may alter its value
fns.c:264: warning: conversion to 'int' from 'long int' may alter its value
fns.c:266: warning: conversion to 'int' from 'long int' may alter its value
fns.c:268: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:270: warning: conversion to 'int' from 'long int' may alter its value
fns.c:272: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:304: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:306: warning: conversion to 'int' from 'Lisp_Object' may alter its value
fns.c:347: warning: conversion to 'int' from 'long int' may alter its value
fns.c:349: warning: conversion to 'int' from 'long int' may alter its value
fns.c:440: warning: conversion to 'int' from 'long unsigned int' may alter its value
fns.c:485: warning: conversion to 'int' from 'long int' may alter its value
fns.c:518: warning: conversion to 'int' from 'Lisp_Object' may alter its value
...

etc.


-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Here's a fun one:

(sxhash 3113)
=> 3113

(sxhash 4543764736473)
=> 3984304601

:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 16:49:51 +0200
> 
> Ok, I think I've found a way to zoom in on this specific problem pretty
> easily.  If we only concern ourselves with conversion from EMACS_INT to
> int, then the following compile command gives us that:
> 
> make -k 2>&1 | egrep 'conversion.*int.*(Lisp_Ob|long)'

Maybe try this on the files I modified today, to see how many problems
I missed.  (I tried this with GCC 4.2.4, but got zero matches from
egrep, probably because the output format of -Wconversion is
completely different.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Maybe try this on the files I modified today, to see how many problems
> I missed.  (I tried this with GCC 4.2.4, but got zero matches from
> egrep, probably because the output format of -Wconversion is
> completely different.

Let's see...

Here's intervals.c:

intervals.c:1938: warning: conversion to 'int' from 'long int' may alter its value
intervals.c:1961: warning: conversion to 'int' from 'long int' may alter its value

Almost totally clean.

editfns.c, however:

make -k 2>&1 | egrep "conversion.*'int'.*(Lisp_Ob|long)"
editfns.c:279: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:281: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:283: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:825: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:997: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:1012: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:1533: warning: conversion to 'int' from 'Lisp_Object' may alter its value
editfns.c:1683: warning: conversion to 'int' from 'long int' may alter its value
editfns.c:1807: warning: conversion to 'int' from 'Lisp_Object' may alter its value
editfns.c:1808: warning: conversion to 'int' from 'Lisp_Object' may alter its value

But I haven't looked at the cases or what they deal with.

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 23 Sep 2010 21:02:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Maybe try this on the files I modified today, to see how many problems
> > I missed.  (I tried this with GCC 4.2.4, but got zero matches from
> > egrep, probably because the output format of -Wconversion is
> > completely different.
> 
> Let's see...
> 
> Here's intervals.c:
> 
> intervals.c:1938: warning: conversion to 'int' from 'long int' may alter its value
> intervals.c:1961: warning: conversion to 'int' from 'long int' may alter its value
> 
> Almost totally clean.

Thanks, fixed.  It was due to return value of a single function.

> editfns.c, however:
> 
> make -k 2>&1 | egrep "conversion.*'int'.*(Lisp_Ob|long)"
> editfns.c:279: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:281: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:283: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:825: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:997: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:1012: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:1533: warning: conversion to 'int' from 'Lisp_Object' may alter its value
> editfns.c:1683: warning: conversion to 'int' from 'long int' may alter its value
> editfns.c:1807: warning: conversion to 'int' from 'Lisp_Object' may alter its value
> editfns.c:1808: warning: conversion to 'int' from 'Lisp_Object' may alter its value

Only the first 3 are related to buffer and string positions (again,
return value of a single function); I fixed them.  The rest are
unrelated: they are about SPECPDL_INDEX and time values.

Thanks.

Re: Buffer size limitation in insdel.c

[Richard Stallman]

    It hides bugs, if nothing else.

Could you please explain what you mean?  Usually an error check does
not hide bugs; rather, it catches them early.

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> CC: emacs-devel@gnu.org
> Date: Fri, 24 Sep 2010 10:09:15 -0400
> 
>     It hides bugs, if nothing else.
> 
> Could you please explain what you mean?  Usually an error check does
> not hide bugs; rather, it catches them early.

Ah, but this is not an error check.  This code prevents us from using
buffers larger than what a 32-bit int can cover, i.e. 2GB.  By
contrast, an Emacs integer is a 64-bit type on 64-bit machines, so we
could potentially have much larger buffers there.

The reason for this code is that many places use an int to hold buffer
positions.  All these places are bugs; they should use EMACS_INT
instead.  This code hides those bugs.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> The reason for this code is that many places use an int to hold buffer
> positions.  All these places are bugs; they should use EMACS_INT
> instead.  This code hides those bugs.

After we've done the initial EMACS_INT/int sweep (shouldn't take more
than a week, right?  :-), we can remove the limitation, try to load a big
file, and see what happens...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Lars Magne Ingebrigtsen <larsi@gnus.org>
> Date: Fri, 24 Sep 2010 16:38:16 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The reason for this code is that many places use an int to hold buffer
> > positions.  All these places are bugs; they should use EMACS_INT
> > instead.  This code hides those bugs.
> 
> After we've done the initial EMACS_INT/int sweep (shouldn't take more
> than a week, right?  :-), we can remove the limitation, try to load a big
> file, and see what happens...

I've now finished reviewing all the sources that Lars didn't handle
and fixing any uses of int for buffer and string positions and sizes.
The only places where I left such code that uses ints unchanged is
when the string or buffer is known in advance to be short.  A typical
example is a string that is a name of a symbol, or a message shown in
the echo area.  I also left alone code that processes the doc strings.

Files I didn't review are: w32*.c files (since the 32-bit Windows
build obviously is not affected), and xselect.c (if it's possible to
have X selections larger than 2GB, then someone who knows more than I
do about this should take a look).

Whether to remove the above limitation should now be a purely
managerial decision.  Stefan and Chong, your call.

P.S.  Note that I went through the sources manually, not with GCC
warnings that Lars used.  So I only fixed code that used ints to refer
to buffer and string positions, not every place where an EMACS_INT is
placed into an int.  And that's in addition to any places I could have
missed, of course.

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> P.S.  Note that I went through the sources manually, not with GCC
> warnings that Lars used.  So I only fixed code that used ints to refer
> to buffer and string positions, not every place where an EMACS_INT is
> placed into an int.  And that's in addition to any places I could have
> missed, of course.

I thought I'd take a quick -Wconversion run-through of the files and
just fix up any string/buffer-related problems it shows, and ignore the
rest...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Lars Magne Ingebrigtsen]

Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

> I thought I'd take a quick -Wconversion run-through of the files and
> just fix up any string/buffer-related problems it shows, and ignore the
> rest...

Geez.  That's not easy to do, because some of the files are so large
that all the non-relevant errors swamp the output, so that you can't see
whether the changes you make fix the problem or not...

-- 
(domestic pets only, the antidote for overdose, milk.)
  larsi@gnus.org * Lars Magne Ingebrigtsen

Re: Buffer size limitation in insdel.c

[Richard Stallman]

    The reason for this code is that many places use an int to hold buffer
    positions.  All these places are bugs; they should use EMACS_INT
    instead.  This code hides those bugs.

I'd say it prevents them from really being bugs.

If people want to fix those places to handle larger buffers,
I have no objections.  However, I doubt your physical memory will
support very many 2-gig strings.

Re: Buffer size limitation in insdel.c

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> CC: emacs-devel@gnu.org
> Date: Sat, 25 Sep 2010 05:40:39 -0400
> 
>     The reason for this code is that many places use an int to hold buffer
>     positions.  All these places are bugs; they should use EMACS_INT
>     instead.  This code hides those bugs.
> 
> I'd say it prevents them from really being bugs.

Such measures are appropriate for a released version, not for a
development version.  In development, you don't sweep bugs under the
carpet.  You let them crash the program, and then you debug them.

> I doubt your physical memory will support very many 2-gig strings.

On my daytime job, I work with a server running RH GNU/Linux.  That
server is a 64-bit machine with 14GB of physical memory (plus the
appropriate amount of swap); it routinely produces log files between
2GB and 5GB.  It is part of my job to browse those logs to diagnose
problems.  I don't like falling back on Less for browsing those logs,
as it is a major annoyance when the logs were compressed into a
.tar.gz or tar.bz2 archive.