Re: [PATCH] Loop Overflow Bug Fix

[Paige DePol <lual@...>]

Roberto Ierusalimschy <roberto@inf.puc-rio.br> wrote:

>>> for i = 2^53 - 10, 2^53 + 10 do print(i) end
>> 
>> [...]
>> 
>> I would guess that people who
>> work with such extremely large numbers would know about the magnitude issues
>> with using very large/small floating values. I know I have learned more
>> about these numeric extremes than I knew before I started this patch! ;)
> 
> The numbers that you have been woried about in your patch (such as
> maxinteger) are a thousand times larger than the "extremely large
> numbers" in this example :-)
> 
> -- Roberto

I know, I had to go look up what the names were for such large numbers! ;)

I'm not "worried" really, it has been an educating experience. The patch
really isn't that significant, it's just an adjustment of the internal logic
used to iterate a for loop. However, if it can get rid of the edge cases,
and make things work as people expect them to, then I think that is probably
worth the change... unless it will break a bunch of existing Lua code?

I did add a little patch to detect insufficient step magnitude, but for each
OP_FORPREP instruction it does add 2 or 4 floating operations and 1 or 2
floating comparisons to check these bounds. Not sure if that is really worth
it given that I'd expect people using such enormous numbers to be aware of
the issues with their numeric precision?

For discussion purposes here is how I am detecting a bad step magnitude,
which is only done for floating-point loops:


In llimits.h:

#define luai_numne(a,b)         ((a)!=(b))


In lvm.c:

#define forstepcheck(ns,ni,nl) \
          (luai_numne(ns, luai_numsub(L, luai_numadd(L, ni, ns), ni)) || \
           luai_numne(ns, luai_numsub(L, nl, luai_numsub(L, nl, ns))))


In lvm.c - luaV_execute() - vmcase(OP_FORPREP):

if (0 < nstep ? forstepcheck(nstep, ninit, nlimit)
              : forstepcheck(nstep, nlimit, ninit))
  luaG_runerror(L, "'for' step magnitude invalid");


Essentially, it just adds and subs from the start and end loop values
depending on the sign of the step value. If the difference from the
original values is not the same as the specified step value then there
is a problem with the precision and the loop will not iterate as expected.

I want to check one last potential optimisation, but at this time I think
the patch works quite well at removing all the edge cases around min/max
integers. The only case remaining that I currently know of is the step
magnitude for floating-point loops... which is also technically fixed.

~Paige