Re: [Tarantool-patches] [PATCH luajit] Fix bit op coercion for shifts in DUALNUM builds.

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Hi, Sergey,

thanks for the patch! LGTM

On 12.03.2025 18:36, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by Junlong Li.
>
> (cherry picked from commit 69bbf3c1b01de8239444b0c430a89fa868978fea)
>
> This is a follow-up to the commit
> 8cd79d198df4b0e14882a663a1673e1308f09899 ("Fix bit op coercion in
> DUALNUM builds."). After removing the coercion from
> `lj_carith_check64()`, the bit shift operation may end in an infinite
> loop in the case of infinite retrying to coerce the second operand from
> number to integer TValue type.
>
> This patch fixes that by unconditionally coercing the second argument in
> the `LJLIB_ASM(bit_lshift)` fast function handler.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#11055
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/fix-bit-shift-dualnum
>
> Note: CI is red due to problems with the integration testing.
> See also:https://github.com/tarantool/tarantool/pull/11220
>
> Related issue:https://github.com/tarantool/tarantool/issues/11055
> ML:https://www.freelists.org/post/luajit/dead-loop-in-bitrshift.
>
> How to build locally for reproducing:
> | cmake -DLUAJIT_NUMMODE=2 -DLUA_USE_APICHECK=ON -DLUA_USE_ASSERT=ON -DCMAKE_BUILD_TYPE=Debug . && make -j
> And run the test like the following:
> | ctest --timeout 1 -R fix-bit-shift-dualnum
>
>   src/lib_bit.c                                 |  2 +-
>   .../fix-bit-shift-dualnum.test.lua            | 27 +++++++++++++++++++
>   2 files changed, 28 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/fix-bit-shift-dualnum.test.lua
>
> diff --git a/src/lib_bit.c b/src/lib_bit.c
> index 6dbaf351..9ac5e645 100644
> --- a/src/lib_bit.c
> +++ b/src/lib_bit.c
> @@ -98,7 +98,7 @@ LJLIB_ASM(bit_lshift)		LJLIB_REC(bit_shift IR_BSHL)
>       x = lj_carith_shift64(x, sh, curr_func(L)->c.ffid - (int)FF_bit_lshift);
>       return bit_result64(L, id, x);
>     }
> -  if (id2) setintV(L->base+1, sh);
> +  setintV(L->base+1, sh);
>     return FFH_RETRY;
>   #else
>     lj_lib_checknumber(L, 1);
> diff --git a/test/tarantool-tests/fix-bit-shift-dualnum.test.lua b/test/tarantool-tests/fix-bit-shift-dualnum.test.lua
> new file mode 100644
> index 00000000..474a365f
> --- /dev/null
> +++ b/test/tarantool-tests/fix-bit-shift-dualnum.test.lua
> @@ -0,0 +1,27 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour for bitshift
> +-- operations in DUALNUM mode.
> +-- See also:
> +--https://www.freelists.org/post/luajit/dead-loop-in-bitrshift.
> +
> +local test = tap.test('fix-bit-shift-dualnum')
> +test:plan(5)
> +
> +-- This produces the number (not integer) `TValue` type for the
> +-- DUALNUM build. If the second parameter of any of the shift
> +-- functions is not an integer in the DUALNUM build, LuaJIT tries
> +-- to convert it to an integer. In the case of a number, it does
> +-- nothing and endlessly retries the call to the fallback
> +-- function.
> +local SHIFT_V = 1 - '0'
> +
> +-- Any of the shift calls below causes the infinite FFH retrying
> +-- loop before the patch.
> +test:ok(bit.arshift(0, SHIFT_V), 0, 'no infifnite loop in bit.arshift')
> +test:ok(bit.lshift(0, SHIFT_V), 0, 'no infifnite loop in bit.lshift')
> +test:ok(bit.rshift(0, SHIFT_V), 0, 'no infifnite loop in bit.rshift')
> +test:ok(bit.rol(0, SHIFT_V), 0, 'no infifnite loop in bit.rol')
> +test:ok(bit.ror(0, SHIFT_V), 0, 'no infifnite loop in bit.ror')
> +
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4347 bytes --]