On 15/02/2010 22:56, Ben Schmidt wrote:
FOUND IT!
Correct behaviour relies on a long overflowing, which it does on 32 bit
machines, but not on 64 bit machines.
From onepage(...) in move.c:
long n;
...
while (n <= curwin->w_height && loff.lnum >= 1)
{
topline_back(&loff);
n += loff.height;
}
if (n <= curwin->w_height) /* at begin of file */
...
From topline_back(...) in move.c:
if (lp->lnum < 1)
lp->height = MAXCOL;
From vim.h:
#if SIZEOF_INT >= 4
# ifdef __MVS__
# define MAXCOL (0x3fffffffL) /* maximum column number, 30 bits */
# else
# define MAXCOL (0x7fffffffL) /* maximum column number, 31 bits */
# endif
#else
In the usual, non __MVS__ case, on 32 or 64 bit machines, we have
MAXCOL=0x7fffffffL. On at least most 32 bit machines,
sizeof(int)==sizeof(long), so n overflows and becomes negative at
n += loff.height when we reach top-of-file, and the if is entered. On at
least some 64 bit machines, sizeof(int)<sizeof(long) (4 vs. 8, it
seems), and so n does not overflow but just gets huge.
The code is clearly wrong, but I'm not 100% sure what the intentions
are.
Perhaps the if should be
if (loff.lnum < 1) /* at begin of file */
?
I don't have time right now to try it, and my build environment isn't
quite set up right yet and not worth the trouble to do so as I'm
thinking of doing an OS reinstallation in a day or two. Can someone else
review the change / test it / suggest something else if it's wrong?
I don't have a 64bit machine to try it on, but wouldn't it be simpler
just to change the declaration of n from long to int?
I thought of that, but suspect it would mess up 16 bit machines. Not
sure if anyone cares about that anymore.
For this case all the variables would be int so it would be consistent.
Although MAXCOL is defined as a 32bit value so you would be back in
the realm of undefined behaviour from signed integer overflow.
It also wouldn't fix the __MVS__ case which will suffer from the same
problem for a different reason.
Alas, I don't have a mainframe at home to try it on either ;-)
And to cross all the ts, the behaviour of overflow with signed integers
is undefined in the C language. But while it can be described as a dodgy
piece of code, for most hardware platforms there shouldn't be a problem.
I doubt this is the only place relying on undefined language behaviour.
I doubt it too. But surely it would be better to have a more sane test
than relying on platform-specific ill-defined oddities.
Indeed. A minimal change, without much knowledge of the meaning and
purpose of most of the structures and functions would be to protect the
addition from happening if it is likely to overflow the result.
Ironically, this is done at least one other place that does something
different. Something like:
while (n <= curwin->w_height && loff.lnum >= 1)
{
topline_back(&loff);
if ( loff.lnum == 0 )
break;
n += loff.height;
}
From the while condition we know n is <= the window height so the test
at the beginning of the file will still hold.
To really be picky, probably MAXCOL is not a good choice of value for
height in the 'above first line' case. Something like LONG_MAX (does
that macro exist?--and on which platforms?) would be more appropriate.
It would return us to relying on the overflow, though, which IMHO isn't
really the best solution.
I haven't looked at all the call sites to be able to say if MAXCOL is a
required value (although a file with 2^32-1 lines would be a fun editing
sessions). In this case INT_MAX would be the one to use if all line
numbers are 32bit only. Ideally INT_MAX would be available on all
platforms but I guess VIM could still be supporting C compilers that are
pre C89 (ANSI C in common parlance).
However, this doesn't really address the core of the problem, the mixing
of long and int integers that can result in different behaviour between
32 and 64 bit CPUs.
Bram probably will have a preferred way of fixing this, I suppose. Now
the problem has been pinpointed it should be pretty trivial for him to
decide on a fix. Probably more helpful for him if someone can test a
proposed fix, though.
Indeed, all those with 64 bit machines step forward (not you Windows
users, your 64bit OS longs are still only 32bits) ...
TTFN
Mike
--
For every action, there is an equal and opposite criticism.
--
You received this message from the "vim_dev" maillist.
For more information, visit http://www.vim.org/maillist.php
