Re: Lua 5.3w2 Test Suite Issues

[Paige DePol <lual@...>]

On Mar 30, 2014, at 7:25 PM, Doug Currie <doug.currie@gmail.com> wrote:

> On Sun, Mar 30, 2014 at 7:22 PM, Paige DePol <lual@serfnet.org> wrote:
> 
> Since you recommend using __builtin_sadd_overflow can you explain what the compiler is doing that is causing the issue in the first place?
>  
> The short answer is that C compilers are allowed to do anything if arithmetic overflows.
> 
> Modern C compilers use this license to aggressively optimize. In this case, since (i <= e) the expression (e-i+1) cannot result in a zero or negative value unless there is an overflow, so the compiler removes the test for negative or zero. The code is expressly trying to detect overflow, but the compiler doesn't know that. With the __builtin function, the compiler is informed of the intention.
> 
> The alternate code 
> 
>   n = e - i;  /* number of elements minus one */
>   if (n > (INT_MAX - 1) || !lua_checkstack(L, n+1))  /* detect arith. overflow */
> 
> avoids the overflow altogether.
> 
> e

Thank you for the information, I was just in the process of making a similar post to answer my own question, but you beat me to it! ;)

Another way of fixing the issue would be to add a single line to lua_checkstack() which checks if 'size' is less than zero, and if so return 0. There is no sanity check in that function, which from what I can tell assumes the 'size' parameter is always a positive value. Once that change is added, the overflow check in unpack() changes from 'n <= 0' to 'n == 0' as the less than zero check is now done by lua_checkstack().

The compiler optimisation does not seem to remove the 'n == 0' check the same way it removed the 'n <= 0' check, which means with these two changes all unpack tests succeed, as do all other tests in the test suite, with the exception of the tests with issues I mentioned in my previous post.

I have attached a patch which changes the overflow check in 'unpack' and adds the sanity check to lua_checkstack(). If my solution has any unforeseen issues please let me know, from what I can tell this should solve the problem for all platforms.

~Paige

Attachment: fix-checkstack.patch
Description: Binary data