* Buffer size limitation in insdel.c @ 2010-09-22 12:06 Eli Zaretskii 2010-09-22 12:12 ` Lars Magne Ingebrigtsen ` (3 more replies) 0 siblings, 4 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-22 12:06 UTC (permalink / raw) To: emacs-devel 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-22 12:06 Buffer size limitation in insdel.c Eli Zaretskii @ 2010-09-22 12:12 ` Lars Magne Ingebrigtsen 2010-09-22 15:20 ` Eli Zaretskii 2010-09-22 12:16 ` David Kastrup ` (2 subsequent siblings) 3 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-22 12:12 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-22 12:12 ` Lars Magne Ingebrigtsen @ 2010-09-22 15:20 ` Eli Zaretskii 0 siblings, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-22 15:20 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-22 12:06 Buffer size limitation in insdel.c Eli Zaretskii 2010-09-22 12:12 ` Lars Magne Ingebrigtsen @ 2010-09-22 12:16 ` David Kastrup 2010-09-23 0:06 ` Stefan Monnier 2010-09-23 0:58 ` Richard Stallman 3 siblings, 0 replies; 33+ messages in thread From: David Kastrup @ 2010-09-22 12:16 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-22 12:06 Buffer size limitation in insdel.c Eli Zaretskii 2010-09-22 12:12 ` Lars Magne Ingebrigtsen 2010-09-22 12:16 ` David Kastrup @ 2010-09-23 0:06 ` Stefan Monnier 2010-09-23 0:58 ` Richard Stallman 3 siblings, 0 replies; 33+ messages in thread From: Stefan Monnier @ 2010-09-23 0:06 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel > "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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-22 12:06 Buffer size limitation in insdel.c Eli Zaretskii ` (2 preceding siblings ...) 2010-09-23 0:06 ` Stefan Monnier @ 2010-09-23 0:58 ` Richard Stallman 2010-09-23 7:55 ` Eli Zaretskii 3 siblings, 1 reply; 33+ messages in thread From: Richard Stallman @ 2010-09-23 0:58 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel { 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 0:58 ` Richard Stallman @ 2010-09-23 7:55 ` Eli Zaretskii 2010-09-23 9:23 ` Leo ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 7:55 UTC (permalink / raw) To: rms; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 7:55 ` Eli Zaretskii @ 2010-09-23 9:23 ` Leo 2010-09-23 9:30 ` Eli Zaretskii 2010-09-23 10:59 ` Lars Magne Ingebrigtsen 2010-09-24 14:09 ` Richard Stallman 2 siblings, 1 reply; 33+ messages in thread From: Leo @ 2010-09-23 9:23 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 9:23 ` Leo @ 2010-09-23 9:30 ` Eli Zaretskii 0 siblings, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 9:30 UTC (permalink / raw) To: Leo; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 7:55 ` Eli Zaretskii 2010-09-23 9:23 ` Leo @ 2010-09-23 10:59 ` Lars Magne Ingebrigtsen 2010-09-23 12:15 ` Eli Zaretskii 2010-09-24 14:09 ` Richard Stallman 2 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 10:59 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 10:59 ` Lars Magne Ingebrigtsen @ 2010-09-23 12:15 ` Eli Zaretskii 2010-09-23 12:18 ` Lars Magne Ingebrigtsen 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 12:15 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 12:15 ` Eli Zaretskii @ 2010-09-23 12:18 ` Lars Magne Ingebrigtsen 2010-09-23 12:47 ` Lars Magne Ingebrigtsen 0 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 12:18 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 12:18 ` Lars Magne Ingebrigtsen @ 2010-09-23 12:47 ` Lars Magne Ingebrigtsen 2010-09-23 13:12 ` Andreas Schwab 2010-09-23 13:22 ` Eli Zaretskii 0 siblings, 2 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 12:47 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 12:47 ` Lars Magne Ingebrigtsen @ 2010-09-23 13:12 ` Andreas Schwab 2010-09-23 13:22 ` Eli Zaretskii 1 sibling, 0 replies; 33+ messages in thread From: Andreas Schwab @ 2010-09-23 13:12 UTC (permalink / raw) To: emacs-devel 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." ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 12:47 ` Lars Magne Ingebrigtsen 2010-09-23 13:12 ` Andreas Schwab @ 2010-09-23 13:22 ` Eli Zaretskii 2010-09-23 13:37 ` Lars Magne Ingebrigtsen 1 sibling, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 13:22 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 13:22 ` Eli Zaretskii @ 2010-09-23 13:37 ` Lars Magne Ingebrigtsen 2010-09-23 14:13 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 13:37 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 13:37 ` Lars Magne Ingebrigtsen @ 2010-09-23 14:13 ` Eli Zaretskii 2010-09-23 14:21 ` Lars Magne Ingebrigtsen 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 14:13 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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)) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:13 ` Eli Zaretskii @ 2010-09-23 14:21 ` Lars Magne Ingebrigtsen 2010-09-23 14:31 ` Lars Magne Ingebrigtsen 2010-09-23 14:41 ` Eli Zaretskii 0 siblings, 2 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 14:21 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:21 ` Lars Magne Ingebrigtsen @ 2010-09-23 14:31 ` Lars Magne Ingebrigtsen 2010-09-23 14:41 ` Eli Zaretskii 1 sibling, 0 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 14:31 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:21 ` Lars Magne Ingebrigtsen 2010-09-23 14:31 ` Lars Magne Ingebrigtsen @ 2010-09-23 14:41 ` Eli Zaretskii 2010-09-23 14:49 ` Lars Magne Ingebrigtsen 1 sibling, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 14:41 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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...). ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:41 ` Eli Zaretskii @ 2010-09-23 14:49 ` Lars Magne Ingebrigtsen 2010-09-23 15:08 ` Lars Magne Ingebrigtsen 2010-09-23 18:56 ` Eli Zaretskii 0 siblings, 2 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 14:49 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:49 ` Lars Magne Ingebrigtsen @ 2010-09-23 15:08 ` Lars Magne Ingebrigtsen 2010-09-23 18:56 ` Eli Zaretskii 1 sibling, 0 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 15:08 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 14:49 ` Lars Magne Ingebrigtsen 2010-09-23 15:08 ` Lars Magne Ingebrigtsen @ 2010-09-23 18:56 ` Eli Zaretskii 2010-09-23 19:02 ` Lars Magne Ingebrigtsen 1 sibling, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 18:56 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 18:56 ` Eli Zaretskii @ 2010-09-23 19:02 ` Lars Magne Ingebrigtsen 2010-09-23 19:22 ` Eli Zaretskii 0 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-23 19:02 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 19:02 ` Lars Magne Ingebrigtsen @ 2010-09-23 19:22 ` Eli Zaretskii 0 siblings, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-23 19:22 UTC (permalink / raw) To: Lars Magne Ingebrigtsen; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-23 7:55 ` Eli Zaretskii 2010-09-23 9:23 ` Leo 2010-09-23 10:59 ` Lars Magne Ingebrigtsen @ 2010-09-24 14:09 ` Richard Stallman 2010-09-24 14:28 ` Eli Zaretskii 2 siblings, 1 reply; 33+ messages in thread From: Richard Stallman @ 2010-09-24 14:09 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-24 14:09 ` Richard Stallman @ 2010-09-24 14:28 ` Eli Zaretskii 2010-09-24 14:38 ` Lars Magne Ingebrigtsen 2010-09-25 9:40 ` Richard Stallman 0 siblings, 2 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-24 14:28 UTC (permalink / raw) To: rms; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-24 14:28 ` Eli Zaretskii @ 2010-09-24 14:38 ` Lars Magne Ingebrigtsen 2010-09-25 13:55 ` Eli Zaretskii 2010-09-25 9:40 ` Richard Stallman 1 sibling, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-24 14:38 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-24 14:38 ` Lars Magne Ingebrigtsen @ 2010-09-25 13:55 ` Eli Zaretskii 2010-09-25 16:26 ` Lars Magne Ingebrigtsen 0 siblings, 1 reply; 33+ messages in thread From: Eli Zaretskii @ 2010-09-25 13:55 UTC (permalink / raw) To: Lars Magne Ingebrigtsen, Stefan Monnier; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-25 13:55 ` Eli Zaretskii @ 2010-09-25 16:26 ` Lars Magne Ingebrigtsen 2010-09-25 16:38 ` Lars Magne Ingebrigtsen 0 siblings, 1 reply; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-25 16:26 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-25 16:26 ` Lars Magne Ingebrigtsen @ 2010-09-25 16:38 ` Lars Magne Ingebrigtsen 0 siblings, 0 replies; 33+ messages in thread From: Lars Magne Ingebrigtsen @ 2010-09-25 16:38 UTC (permalink / raw) To: emacs-devel 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-24 14:28 ` Eli Zaretskii 2010-09-24 14:38 ` Lars Magne Ingebrigtsen @ 2010-09-25 9:40 ` Richard Stallman 2010-09-25 10:00 ` Eli Zaretskii 1 sibling, 1 reply; 33+ messages in thread From: Richard Stallman @ 2010-09-25 9:40 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Buffer size limitation in insdel.c 2010-09-25 9:40 ` Richard Stallman @ 2010-09-25 10:00 ` Eli Zaretskii 0 siblings, 0 replies; 33+ messages in thread From: Eli Zaretskii @ 2010-09-25 10:00 UTC (permalink / raw) To: rms; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2010-09-25 16:38 UTC | newest] Thread overview: 33+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-22 12:06 Buffer size limitation in insdel.c Eli Zaretskii 2010-09-22 12:12 ` Lars Magne Ingebrigtsen 2010-09-22 15:20 ` Eli Zaretskii 2010-09-22 12:16 ` David Kastrup 2010-09-23 0:06 ` Stefan Monnier 2010-09-23 0:58 ` Richard Stallman 2010-09-23 7:55 ` Eli Zaretskii 2010-09-23 9:23 ` Leo 2010-09-23 9:30 ` Eli Zaretskii 2010-09-23 10:59 ` Lars Magne Ingebrigtsen 2010-09-23 12:15 ` Eli Zaretskii 2010-09-23 12:18 ` Lars Magne Ingebrigtsen 2010-09-23 12:47 ` Lars Magne Ingebrigtsen 2010-09-23 13:12 ` Andreas Schwab 2010-09-23 13:22 ` Eli Zaretskii 2010-09-23 13:37 ` Lars Magne Ingebrigtsen 2010-09-23 14:13 ` Eli Zaretskii 2010-09-23 14:21 ` Lars Magne Ingebrigtsen 2010-09-23 14:31 ` Lars Magne Ingebrigtsen 2010-09-23 14:41 ` Eli Zaretskii 2010-09-23 14:49 ` Lars Magne Ingebrigtsen 2010-09-23 15:08 ` Lars Magne Ingebrigtsen 2010-09-23 18:56 ` Eli Zaretskii 2010-09-23 19:02 ` Lars Magne Ingebrigtsen 2010-09-23 19:22 ` Eli Zaretskii 2010-09-24 14:09 ` Richard Stallman 2010-09-24 14:28 ` Eli Zaretskii 2010-09-24 14:38 ` Lars Magne Ingebrigtsen 2010-09-25 13:55 ` Eli Zaretskii 2010-09-25 16:26 ` Lars Magne Ingebrigtsen 2010-09-25 16:38 ` Lars Magne Ingebrigtsen 2010-09-25 9:40 ` Richard Stallman 2010-09-25 10:00 ` Eli Zaretskii
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.