scroll-conservatively overflow

scroll-conservatively overflow

[Juanma Barranquero]

I like scrolling one line at a time. I have

(setq scroll-step 0
      scroll-conservatively most-positive-fixnum)

on my .emacs because scroll-step's documentation recommends setting
scroll-conservatively to "a large value" and most-positive-fixnum seems
less arbitrary than 10000 or 65535 or whatever.

That used to work; now it doesn't (although I'm not sure it is because
of a recent change or I just took a time to notice), because
scroll-conservatively gets multiplied by FRAME_LINE_HEIGHT (f) in a
couple of places, and that causes an overflow.

I have two options: either cutting scroll-conservatively down to a
manageable size before any use of it, or patching the docstring for
scroll-step to say that a large, but *reasonable*, value should be used.

I favor patching try_scrolling, because I don't even want to think how
to describe "a reasonable value" on the docstring :) and because we should
protect against unintended overflows like this one (the users can have
set scroll-conservatively to a big value on their .emacs and would get
wrong behaviour when switching to 21.X, X > 3).

So, if no one opposes, I'll install the attached patch.

                                                                Juanma



--- xdisp.c.orig	2004-04-14 22:33:44.000000000 +0200
+++ xdisp.c	2004-04-16 11:39:38.000000000 +0200
@@ -10881,2 +10881,8 @@
 
+  /* Force scroll_conservatively to have a reasonable value so it doesn't cause
+     an overflow while computing how much to scroll.  */
+  if (scroll_conservatively)
+      scroll_conservatively = min (scroll_conservatively,
+                                   MOST_POSITIVE_FIXNUM / FRAME_LINE_HEIGHT (f));
+
   /* Compute how much we should try to scroll maximally to bring point

Re: scroll-conservatively overflow

[Kim F. Storm]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> I like scrolling one line at a time. I have
> 
> (setq scroll-step 0
>       scroll-conservatively most-positive-fixnum)
> 
> on my .emacs because scroll-step's documentation recommends setting
> scroll-conservatively to "a large value" and most-positive-fixnum seems
> less arbitrary than 10000 or 65535 or whatever.
> 
> That used to work; now it doesn't (although I'm not sure it is because
> of a recent change or I just took a time to notice), because
> scroll-conservatively gets multiplied by FRAME_LINE_HEIGHT (f) in a
> couple of places, and that causes an overflow.

That change was made almost a year ago...

I changed some things related to scroll a few days ago which may have
made this more visible -- I don't know.

> So, if no one opposes, I'll install the attached patch.

Please install it.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: scroll-conservatively overflow

[Romain Francoise]

no-spam@cua.dk (Kim F. Storm) writes:

> I changed some things related to scroll a few days ago which may have
> made this more visible -- I don't know.

About scrolling: I've been bitten by a bug for some weeks now, I
originally thought it was specific to the multi-tty code but I
discovered this morning that it's not.  (A fresh checkout from CVS HEAD
this morning exhibited the same issues.)

I see it in two different situations:

 1. In Gnus, when I scroll a long article buffer (several pages) by
    repeatedly hitting RET to scroll one line at a time, scrolling stops
    on truncated lines.  For example, when I read an article that
    contains one very long line (e.g. a URL), it scrolls okay until the
    long line is the first line in the Article buffer.  I hit RET again
    and the first part of the line scrolls away.  I still see in the
    buffer the continuation part of the long (truncated) line, and if I
    hit RET again, nothing happens, it doesn't scroll more.  I have
    traced the call back in the Lisp code and it shows that the
    `scroll-up' builtin function returns nil as usual but doesn't have
    any effect on the buffer.  I can keep hitting RET to no avail.

    Scrolling works fine until the "End of buffer" message as usual when
    the article does not contain long lines; it also works fine if I
    resize the window so that lines don't get truncated.  It also works
    fine if I scroll past long lines in the article with SPC, then use
    RET to scroll one line at a time.

    This bug is reproducible in all instances of Emacs, at once.

 2. In Dired, I get "End of buffer" errors when moving over truncated
    lines, from the top to the bottom of a buffer (with `n').  For
    example, if my Dired buffer contains, in the middle of a long
    listing:

    -rw-r--r--    1 romain   romain       2047 Feb 26  2003 bpf_dump.c
    lrwxr-xr-x    1 romain   romain         22 Nov 11 12:26 bpf_filter.c -> ./bpf/net/bpf_filter.c
    -rw-r--r--    1 romain   romain       4966 Feb 26  2003 bpf_image.c

    (The middle line is truncated after "-> ./bp", I see:

     bpf_filter.c -> ./bp\
f/net/bpf_filter.c

    which might not be the case for you depending on your window width.)

    I move the cursor down to the `b' in "bpf_dump.c".  I then use `n'
    which moves the point to the `b' in "bpf_filter.c".  If I then use
    `n' to move down to the next line over the symlink line, the bell is
    rung, and I get a "End of buffer" message in the minibuffer, which
    is obviously wrong since the buffer continues..  After the error,
    the point is on the last `c' in "bpf_image.c", instead of being on
    the `b', but I think this is because of some Dired magic which gets
    confused by the error.

    The fun part is that it does not only happen in Dired buffers: in
    this very Message buffer where I'm composing this post, I can move up
    to the top with C-p, but when I move down to the bottom with C-n, I
    get an "End of buffer" error over the pasted Dired line.

    This bug does not appear in a fresh Emacs session just after I
    launch it, it appears only after I've used Emacs a bit.

This only happens in console frames, in X frames I never managed to
reproduce these bugs.  They are _very_ annoying since it makes the
display flash the bell every once in a while even though no error
occurred...

I can provide more information if needed.

-- 
Romain Francoise <romain@orebokech.com> | You know that old saying,
it's a miracle -- http://orebokech.com/ | that you always hurt the ones
                                        | you love? Well it works both
                                        | ways.

Re: scroll-conservatively overflow

[Richard Stallman]

    > That used to work; now it doesn't (although I'm not sure it is because
    > of a recent change or I just took a time to notice), because
    > scroll-conservatively gets multiplied by FRAME_LINE_HEIGHT (f) in a
    > couple of places, and that causes an overflow.

    That change was made almost a year ago...

Multiplying by FRAME_LINE_HEIGHT is right in principle, but the
overflow is a bug.  Perhaps we should use long long for that
multiplication, and if the result is bigger than the largest positive
integer, reduce it to the largest positive integer.

Re: scroll-conservatively overflow

[Juanma Barranquero]

On Sat, 17 Apr 2004 15:42:07 -0400, Richard Stallman <rms@gnu.org> wrote:

> Perhaps we should use long long for that
> multiplication, and if the result is bigger than the largest positive
> integer, reduce it to the largest positive integer.

Well, that's almost what my patch does, with the difference that for
large values of scroll-conservatively it won't produce the largest
positive integer, but 

  int(most-positive-fixnum / FRAME_LINE_HEIGHT (f)) * FRAME_LINE_HEIGHT (f)

but the two are equivalent for all practical purposes.

                                                           /L/e/k/t/u

Re: scroll-conservatively overflow

[Kim F. Storm]

Juanma Barranquero <lektu@mi.madritel.es> writes:

> On Sat, 17 Apr 2004 15:42:07 -0400, Richard Stallman <rms@gnu.org> wrote:
> 
> > Perhaps we should use long long for that
> > multiplication, and if the result is bigger than the largest positive
> > integer, reduce it to the largest positive integer.
> 
> Well, that's almost what my patch does, with the difference that for
> large values of scroll-conservatively it won't produce the largest
> positive integer, but 
> 
>   int(most-positive-fixnum / FRAME_LINE_HEIGHT (f)) * FRAME_LINE_HEIGHT (f)
> 
> but the two are equivalent for all practical purposes.

Yes, IMO your patch is good enough in practice.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: scroll-conservatively overflow

[Richard Stallman]

    > Perhaps we should use long long for that
    > multiplication, and if the result is bigger than the largest positive
    > integer, reduce it to the largest positive integer.

    Well, that's almost what my patch does,

Ok, thanks.

Re: scroll-conservatively overflow

[Romain Francoise]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

Romain Francoise <romain@orebokech.com> writes:

> About scrolling: I've been bitten by a bug for some weeks now, I
> originally thought it was specific to the multi-tty code but I
> discovered this morning that it's not.  (A fresh checkout from CVS HEAD
> this morning exhibited the same issues.)

I have finally found enough time to track it down.  The change
responsible for all my scrolling problems is Richard's change to
indent.c on March 2nd:

2004-03-02  Richard M. Stallman  <rms@gnu.org>

	* indent.c (compute_motion): Save vpos in prev_vpos, like hpos etc.

Reverting this change (see attached patch) solves all issues in Dired,
Gnus, etc.

I have tried to come up with a fix but failed.  I don't really
understand why this change was necessary, it adds complexity and does
not fix anything as far as I can see.  If it was just a code cleanup,
maybe the best would be to revert it for the time being.

-- 
Romain Francoise <romain@orebokech.com> | This is a man's man's man's
it's a miracle -- http://orebokech.com/ | world. --James Brown

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: emacs-indent-fix.diff --]
[-- Type: text/x-patch, Size: 2421 bytes --]

Index: src/indent.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/indent.c,v
retrieving revision 1.160
diff -c -r1.160 indent.c
*** src/indent.c	5 Apr 2004 21:41:08 -0000	1.160
--- src/indent.c	30 Apr 2004 11:27:23 -0000
***************
*** 1197,1202 ****
--- 1197,1203 ----
      = (INTEGERP (current_buffer->selective_display)
         ? XINT (current_buffer->selective_display)
         : !NILP (current_buffer->selective_display) ? -1 : 0);
+   int prev_hpos = 0;
    int selective_rlen
      = (selective && dp && VECTORP (DISP_INVIS_VECTOR (dp))
         ? XVECTOR (DISP_INVIS_VECTOR (dp))->size : 0);
***************
*** 1224,1231 ****
    int wide_column_end_hpos = 0;
    int prev_pos;			/* Previous buffer position.  */
    int prev_pos_byte;		/* Previous buffer position.  */
-   int prev_hpos = 0;
-   int prev_vpos = 0;
    int contin_hpos;		/* HPOS of last column of continued line.  */
    int prev_tab_offset;		/* Previous tab offset.  */
  
--- 1225,1230 ----
***************
*** 1274,1280 ****
  		  pos = prev_pos;
  		  pos_byte = prev_pos_byte;
  		  hpos = prev_hpos;
- 		  vpos = prev_vpos;
  		  tab_offset = prev_tab_offset;
  		}
  	      break;
--- 1273,1278 ----
***************
*** 1384,1390 ****
  		  if (pos >= next_boundary)
  		    next_boundary = pos + 1;
  		  prev_hpos = width;
- 		  prev_vpos = vpos;
  		  prev_tab_offset = tab_offset;
  		}
  	    }
--- 1382,1387 ----
***************
*** 1417,1423 ****
  	  pos = prev_pos;
  	  pos_byte = prev_pos_byte;
  	  hpos = prev_hpos;
- 	  vpos = prev_vpos;
  	  tab_offset = prev_tab_offset;
  
  	  /* NOTE on contin_hpos, hpos, and prev_hpos.
--- 1414,1419 ----
***************
*** 1438,1443 ****
--- 1434,1443 ----
  	      hpos = contin_hpos;
  	      vpos = vpos - 1;
  	    }
+ 	  else if (c == '\n')
+ 	    /* If previous character is NEWLINE,
+ 	       set VPOS back to previous line */
+ 	    vpos = vpos - 1;
  	  break;
  	}
  
***************
*** 1455,1461 ****
  	      pos = prev_pos;
  	      pos_byte = prev_pos_byte;
  	      hpos = prev_hpos;
- 	      vpos = prev_vpos;
  	      tab_offset = prev_tab_offset;
  	    }
  	  break;
--- 1455,1460 ----
***************
*** 1464,1470 ****
  	break;
  
        prev_hpos = hpos;
-       prev_vpos = vpos;
        prev_pos = pos;
        prev_pos_byte = pos_byte;
        wide_column_end_hpos = 0;
--- 1463,1468 ----

[-- Attachment #3: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/emacs-devel

Re: scroll-conservatively overflow

[Kim F. Storm]

Romain Francoise <romain@orebokech.com> writes:

> Romain Francoise <romain@orebokech.com> writes:
> 
> > About scrolling: I've been bitten by a bug for some weeks now, I
> > originally thought it was specific to the multi-tty code but I
> > discovered this morning that it's not.  (A fresh checkout from CVS HEAD
> > this morning exhibited the same issues.)
> 
> I have finally found enough time to track it down.  The change
> responsible for all my scrolling problems is Richard's change to
> indent.c on March 2nd:
> 
> 2004-03-02  Richard M. Stallman  <rms@gnu.org>
> 
> 	* indent.c (compute_motion): Save vpos in prev_vpos, like hpos etc.
> 
> Reverting this change (see attached patch) solves all issues in Dired,
> Gnus, etc.
> 
> I have tried to come up with a fix but failed.  I don't really
> understand why this change was necessary, it adds complexity and does
> not fix anything as far as I can see.  If it was just a code cleanup,
> maybe the best would be to revert it for the time being.

The change was made in response to the following bug report:
The fix was obviously not correct.


Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

Place the following in a file named `/tmp/x'
======cut here=====
Barf

        Whee!

Yadda
======cut here=====

(the above contains three lines, no tabs, and no trailing spaces, for
a total of 27 characters)

Now do the following:

* start `emacs -q --no-site-file'

* find the file: C-x C-f /tmp/x RET

* turn on selective-display: C-u C-x $

* attempt to move point forward: C-n C-n

Note that the cursor did not appear to move after the second C-n.
That's the bug; it should have appeared on the third visible line
(right before the `Y').

* sanity check: C-x =

You should see point 6 of 27.

* again attempt to move point forward: C-n

* again sanity check: C-x =

It's still at 6 of 27.

If it matters, you can "un-stick" point by typing C-f, *then* typing
C-n; in that case, point indeed winds up on the third visible line,
where I'd expect it to.


-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: scroll-conservatively overflow

[Richard Stallman]

    2004-03-02  Richard M. Stallman  <rms@gnu.org>

	    * indent.c (compute_motion): Save vpos in prev_vpos, like hpos etc.

    Reverting this change (see attached patch) solves all issues in Dired,
    Gnus, etc.

    I have tried to come up with a fix but failed.  I don't really
    understand why this change was necessary, it adds complexity and does
    not fix anything as far as I can see.

I made that change to fix a specific bug.  That function was returning
incorrect results in a certain case.  I don't remember the test case,
unfortunately.

This change also makes the function cleaner because it handles vpos
like the various other values.

Apparently the new code has another bug.  That often happens.  Could
you figure out precisely what the bug is?  Then we could find the
right fix.

Re: scroll-conservatively overflow

[Romain Francoise]

Richard Stallman <rms@gnu.org> writes:

> I made that change to fix a specific bug.  That function was returning
> incorrect results in a certain case.  I don't remember the test case,
> unfortunately.

Kim F. Storm kindly provided me with the original bug report so I've
been able to reproduce it.

> This change also makes the function cleaner because it handles vpos
> like the various other values.

Yes; I wasn't convinced at first that doing this was necessary, but now
that I have looked into it in more detail I agree that it is cleaner
this way.

> Apparently the new code has another bug.  That often happens.  Could
> you figure out precisely what the bug is?  Then we could find the
> right fix.

I have spent some time reading the code and found the problem: you had
overlooked a case where vpos needs to be saved in prev_vpos when dealing
with continuation lines.  Your change did not save the value even though
it had been incremented to take into account the fact that the line is
continued on another line, thus making the next iteration restore vpos
from prev_vpos with a wrong value, which caused all my scrolling
problems in Gnus and Dired.

I suggest the following patch against CVS which merely saves vpos in
prev_vpos in that case, and that fixes the various problems I was
having.  I have also checked that the original bug is still fixed with
that change.  Could you please install it?

Index: src/indent.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/indent.c,v
retrieving revision 1.160
diff -c -r1.160 indent.c
*** src/indent.c	5 Apr 2004 21:41:08 -0000	1.160
--- src/indent.c	2 May 2004 11:54:42 -0000
***************
*** 1407,1412 ****
--- 1407,1413 ----
  	      vpos++;
  	      contin_hpos = prev_hpos;
  	      prev_hpos = 0;
+ 	      prev_vpos = vpos;
  	    }
  	}

Suggested changelog entry:

2004-05-02  Romain Francoise  <romain@orebokech.com>  (tiny change)

	* indent.c (compute_motion): Save vpos in prev_vpos when dealing
	with continuation lines, too.

Thanks,

-- 
Romain Francoise <romain@orebokech.com> | This is a man's man's man's
it's a miracle -- http://orebokech.com/ | world. --James Brown

Re: scroll-conservatively overflow

[Stefan Monnier]

> 2004-05-02  Romain Francoise  <romain@orebokech.com>  (tiny change)

> 	* indent.c (compute_motion): Save vpos in prev_vpos when dealing
> 	with continuation lines, too.

Thank you,
Installed,


        Stefan

Re: scroll-conservatively overflow

[Richard Stallman]

Thanks for finding the new bug.