Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library
@ 2024-10-02  8:09 Sergey Kaplun via Tarantool-patches
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02  8:09 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

This patchset fixes a bunch of misbehaviours for the 64-bit-wide
operands in the built-in `bit` library.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1079-fix-64-bitshift-folds
Related issues:
* https://github.com/tarantool/tarantool/issues/10199
* https://github.com/LuaJIT/LuaJIT/issues/1079
* https://github.com/LuaJIT/LuaJIT/issues/1273

Mike Pall (2):
  Fix bit op coercion in DUALNUM builds.
  FFI: Fix 64 bit shift fold rules.

 src/lj_carith.c                               |  4 +-
 src/lj_opt_fold.c                             |  8 +-
 .../lj-1079-fix-64-bitshift-folds.test.lua    | 74 +++++++++++++++++++
 .../lj-1273-dualnum-bit-coercion.test.lua     | 19 +++++
 4 files changed, 98 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
 create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua

-- 
2.46.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
  2024-10-02  8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
@ 2024-10-02  8:09 ` Sergey Kaplun via Tarantool-patches
  2024-10-08 10:12   ` Sergey Bronnikov via Tarantool-patches
  2024-10-11 19:08   ` Maxim Kokryashkin via Tarantool-patches
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
  2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
  2 siblings, 2 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02  8:09 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit f5fd22203eadf57ccbaa4a298010d23974b22fc0)

The `lj_carith_check64()` function coerces the given number value to the
32-bit wide value. In this case, the 64-bit-wide operands will lose
upper bits.

This patch removes the excess coercion for the DUALNUM mode.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#10199
---
 src/lj_carith.c                               |  4 +---
 .../lj-1273-dualnum-bit-coercion.test.lua     | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua

diff --git a/src/lj_carith.c b/src/lj_carith.c
index 90b3220f..eb56d552 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -349,9 +349,7 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
   if (LJ_LIKELY(tvisint(o))) {
     return (uint32_t)intV(o);
   } else {
-    int32_t i = lj_num2bit(numV(o));
-    if (LJ_DUALNUM) setintV(o, i);
-    return (uint32_t)i;
+    return (uint32_t)lj_num2bit(numV(o));
   }
 }
 
diff --git a/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
new file mode 100644
index 00000000..e83dbbcd
--- /dev/null
+++ b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
@@ -0,0 +1,19 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour on operating
+-- for 64-bit operands in built-in `bit` library in DUALNUM mode.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/1273.
+
+local test = tap.test('lj-1273-dualnum-bit-coercion')
+test:plan(1)
+
+local bit = require('bit')
+
+-- The cdata value for 2 ^ 33.
+local EXPECTED = 2 ^ 33 + 0LL
+-- Same value is used as mask for `bit.band()`.
+local MASK = EXPECTED
+
+test:is(bit.band(2 ^ 33, MASK), EXPECTED, 'correct bit.band result')
+
+test:done(true)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
  2024-10-02  8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
@ 2024-10-02  8:09 ` Sergey Kaplun via Tarantool-patches
  2024-10-08 12:07   ` Sergey Bronnikov via Tarantool-patches
  2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
  2 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02  8:09 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)

For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
argument is truncated down to 32 bits, which leads to incorrect results
if the right argument is >= 32.

Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
since this case branch is unreachable, it is harmless for now.

This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
preserve IR semantics.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#10199
---
 src/lj_opt_fold.c                             |  8 +-
 .../lj-1079-fix-64-bitshift-folds.test.lua    | 74 +++++++++++++++++++
 2 files changed, 78 insertions(+), 4 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua

diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index e2171e1b..2702f79f 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -382,10 +382,10 @@ static uint64_t kfold_int64arith(jit_State *J, uint64_t k1, uint64_t k2,
   case IR_BOR: k1 |= k2; break;
   case IR_BXOR: k1 ^= k2; break;
   case IR_BSHL: k1 <<= (k2 & 63); break;
-  case IR_BSHR: k1 = (int32_t)((uint32_t)k1 >> (k2 & 63)); break;
-  case IR_BSAR: k1 >>= (k2 & 63); break;
-  case IR_BROL: k1 = (int32_t)lj_rol((uint32_t)k1, (k2 & 63)); break;
-  case IR_BROR: k1 = (int32_t)lj_ror((uint32_t)k1, (k2 & 63)); break;
+  case IR_BSHR: k1 >>= (k2 & 63); break;
+  case IR_BSAR: k1 = (uint64_t)((int64_t)k1 >> (k2 & 63)); break;
+  case IR_BROL: k1 = lj_rol(k1, (k2 & 63)); break;
+  case IR_BROR: k1 = lj_ror(k1, (k2 & 63)); break;
   default: lj_assertJ(0, "bad IR op %d", op); break;
   }
 #else
diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
new file mode 100644
index 00000000..6cc0b319
--- /dev/null
+++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
@@ -0,0 +1,74 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour on folding
+-- for bitshift operations.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/1079.
+
+local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+local bit = require('bit')
+
+test:plan(4)
+
+-- Generic function for `bit.ror()`, `bit.rol()`.
+local function bitop_rotation(bitop)
+  local r = {}
+  for i = 1, 4 do
+    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+    local int64 = bit.band(i, 7LL)
+    r[i] = tonumber(bitop(int64, 32))
+  end
+  return r
+end
+
+-- Similar function for `bit.rshift()`.
+local function bitop_rshift_signed()
+  local r = {}
+  for i = 1, 4 do
+    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+    -- XXX: Use `-i` instead of `i` to prevent other folding due
+    -- to IR difference so the IRs don't match fold rule mask.
+    -- (-i & 7LL) < 1 << 32 => result == 0.
+    local int64 = bit.band(-i, 7LL)
+    r[i] = tonumber(bit.rshift(int64, 32))
+  end
+  return r
+end
+
+-- A little bit different example, which leads to the assertion
+-- failure due to the incorrect recording.
+local function bitop_rshift_huge()
+  local r = {}
+  for i = 1, 4 do
+    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+    -- XXX: Need to use cast to the int64_t via `+ 0LL`, see the
+    -- documentation [1] for the details.
+    -- [1]: https://bitop.luajit.org/semantics.html
+    local int64 = bit.band(2 ^ 33 + i, 2 ^ 33 + 0LL)
+    r[i] = tonumber(bit.rshift(int64, 32))
+  end
+  return r
+end
+
+local function test_64bitness(subtest, payload_func, bitop)
+  subtest:plan(1)
+
+  jit.off()
+  jit.flush()
+  local results_joff = payload_func(bitop)
+  jit.on()
+  -- Reset hotcounters.
+  jit.opt.start('hotloop=1')
+  local results_jon = payload_func(bitop)
+  subtest:is_deeply(results_jon, results_joff,
+                    'same results for VM and JIT for ' .. subtest.name)
+end
+
+test:test('rol', test_64bitness, bitop_rotation, bit.rol)
+test:test('ror', test_64bitness, bitop_rotation, bit.ror)
+test:test('rshift signed', test_64bitness, bitop_rshift_signed)
+test:test('rshift huge',   test_64bitness, bitop_rshift_huge)
+
+test:done(true)
-- 
2.46.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
@ 2024-10-08 10:12   ` Sergey Bronnikov via Tarantool-patches
  2024-10-11 19:08   ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-08 10:12 UTC (permalink / raw)
  To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches

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

Hello, Sergey,

thanks for the patch! LGTM

On 02.10.2024 11:09, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit f5fd22203eadf57ccbaa4a298010d23974b22fc0)
>
> The `lj_carith_check64()` function coerces the given number value to the
> 32-bit wide value. In this case, the 64-bit-wide operands will lose
> upper bits.
>
> This patch removes the excess coercion for the DUALNUM mode.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
>   src/lj_carith.c                               |  4 +---
>   .../lj-1273-dualnum-bit-coercion.test.lua     | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+), 3 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
>
> diff --git a/src/lj_carith.c b/src/lj_carith.c
> index 90b3220f..eb56d552 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -349,9 +349,7 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
>     if (LJ_LIKELY(tvisint(o))) {
>       return (uint32_t)intV(o);
>     } else {
> -    int32_t i = lj_num2bit(numV(o));
> -    if (LJ_DUALNUM) setintV(o, i);
> -    return (uint32_t)i;
> +    return (uint32_t)lj_num2bit(numV(o));
>     }
>   }
>   
> diff --git a/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
> new file mode 100644
> index 00000000..e83dbbcd
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
> @@ -0,0 +1,19 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour on operating
> +-- for 64-bit operands in built-in `bit` library in DUALNUM mode.
> +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1273.
> +
> +local test = tap.test('lj-1273-dualnum-bit-coercion')
> +test:plan(1)
> +
> +local bit = require('bit')
> +
> +-- The cdata value for 2 ^ 33.
> +local EXPECTED = 2 ^ 33 + 0LL
> +-- Same value is used as mask for `bit.band()`.
> +local MASK = EXPECTED
> +
> +test:is(bit.band(2 ^ 33, MASK), EXPECTED, 'correct bit.band result')
> +
> +test:done(true)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
@ 2024-10-08 12:07   ` Sergey Bronnikov via Tarantool-patches
  2024-10-08 14:24     ` Sergey Kaplun via Tarantool-patches
  0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-08 12:07 UTC (permalink / raw)
  To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches

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

Hello, Sergey,

thanks for the patch! Please see my comments below.

On 02.10.2024 11:09, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)
>
> For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
> argument is truncated down to 32 bits, which leads to incorrect results
> if the right argument is >= 32.
typo: is >= 2,147,483,647
>
> Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
> since this case branch is unreachable, it is harmless for now.
>
> This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
> preserve IR semantics.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
>   src/lj_opt_fold.c                             |  8 +-
>   .../lj-1079-fix-64-bitshift-folds.test.lua    | 74 +++++++++++++++++++
>   2 files changed, 78 insertions(+), 4 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
>
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index e2171e1b..2702f79f 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -382,10 +382,10 @@ static uint64_t kfold_int64arith(jit_State *J, uint64_t k1, uint64_t k2,
>     case IR_BOR: k1 |= k2; break;
>     case IR_BXOR: k1 ^= k2; break;
>     case IR_BSHL: k1 <<= (k2 & 63); break;
> -  case IR_BSHR: k1 = (int32_t)((uint32_t)k1 >> (k2 & 63)); break;
> -  case IR_BSAR: k1 >>= (k2 & 63); break;
> -  case IR_BROL: k1 = (int32_t)lj_rol((uint32_t)k1, (k2 & 63)); break;
> -  case IR_BROR: k1 = (int32_t)lj_ror((uint32_t)k1, (k2 & 63)); break;
> +  case IR_BSHR: k1 >>= (k2 & 63); break;
> +  case IR_BSAR: k1 = (uint64_t)((int64_t)k1 >> (k2 & 63)); break;
> +  case IR_BROL: k1 = lj_rol(k1, (k2 & 63)); break;
> +  case IR_BROR: k1 = lj_ror(k1, (k2 & 63)); break;
>     default: lj_assertJ(0, "bad IR op %d", op); break;
>     }
>   #else
> diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> new file mode 100644
> index 00000000..6cc0b319
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> @@ -0,0 +1,74 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour on folding
> +-- for bitshift operations.
> +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1079.
> +
> +local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local bit = require('bit')
> +
> +test:plan(4)
> +
> +-- Generic function for `bit.ror()`, `bit.rol()`.
> +local function bitop_rotation(bitop)

I would rename arg `bitop` to `bitop_func` to highlight the type

of the value.

> +  local r = {}
> +  for i = 1, 4 do
> +    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> +    local int64 = bit.band(i, 7LL)
> +    r[i] = tonumber(bitop(int64, 32))
please add comments about magic constants here and below
> +  end
> +  return r
> +end
> +
> +-- Similar function for `bit.rshift()`.
> +local function bitop_rshift_signed()
> +  local r = {}
> +  for i = 1, 4 do
> +    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> +    -- XXX: Use `-i` instead of `i` to prevent other folding due
> +    -- to IR difference so the IRs don't match fold rule mask.
> +    -- (-i & 7LL) < 1 << 32 => result == 0.
> +    local int64 = bit.band(-i, 7LL)
> +    r[i] = tonumber(bit.rshift(int64, 32))
> +  end
> +  return r
> +end
> +
> +-- A little bit different example, which leads to the assertion
> +-- failure due to the incorrect recording.
> +local function bitop_rshift_huge()
> +  local r = {}
> +  for i = 1, 4 do
> +    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> +    -- XXX: Need to use cast to the int64_t via `+ 0LL`, see the
> +    -- documentation [1] for the details.
> +    -- [1]:https://bitop.luajit.org/semantics.html
> +    local int64 = bit.band(2 ^ 33 + i, 2 ^ 33 + 0LL)
> +    r[i] = tonumber(bit.rshift(int64, 32))
> +  end
> +  return r
> +end
> +
> +local function test_64bitness(subtest, payload_func, bitop)
> +  subtest:plan(1)
> +
> +  jit.off()
> +  jit.flush()
> +  local results_joff = payload_func(bitop)
> +  jit.on()
> +  -- Reset hotcounters.
> +  jit.opt.start('hotloop=1')
> +  local results_jon = payload_func(bitop)
> +  subtest:is_deeply(results_jon, results_joff,
> +                    'same results for VM and JIT for ' .. subtest.name)
> +end
> +
> +test:test('rol', test_64bitness, bitop_rotation, bit.rol)
> +test:test('ror', test_64bitness, bitop_rotation, bit.ror)
> +test:test('rshift signed', test_64bitness, bitop_rshift_signed)
> +test:test('rshift huge',   test_64bitness, bitop_rshift_huge)
have you added additional whitespaces intentionally?
> +
> +test:done(true)

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
  2024-10-08 12:07   ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-08 14:24     ` Sergey Kaplun via Tarantool-patches
  2024-10-09 14:29       ` Sergey Bronnikov via Tarantool-patches
  2024-10-11 19:12       ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 2 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-08 14:24 UTC (permalink / raw)
  To: Sergey Bronnikov; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 08.10.24, Sergey Bronnikov wrote:
> Hello, Sergey,
> 
> thanks for the patch! Please see my comments below.
> 
> On 02.10.2024 11:09, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)
> >
> > For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
> > argument is truncated down to 32 bits, which leads to incorrect results
> > if the right argument is >= 32.
> typo: is >= 2,147,483,647

Nice catch! Thanks! :)
I replaced with 2^32.

> >
> > Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
> > since this case branch is unreachable, it is harmless for now.
> >
> > This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
> > preserve IR semantics.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#10199
> > ---
> >   src/lj_opt_fold.c                             |  8 +-
> >   .../lj-1079-fix-64-bitshift-folds.test.lua    | 74 +++++++++++++++++++
> >   2 files changed, 78 insertions(+), 4 deletions(-)
> >   create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> >
> > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> > index e2171e1b..2702f79f 100644
> > --- a/src/lj_opt_fold.c
> > +++ b/src/lj_opt_fold.c

<snipped>

> > diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> > new file mode 100644
> > index 00000000..6cc0b319
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> > @@ -0,0 +1,74 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT misbehaviour on folding
> > +-- for bitshift operations.
> > +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1079.
> > +
> > +local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +local bit = require('bit')
> > +
> > +test:plan(4)
> > +
> > +-- Generic function for `bit.ror()`, `bit.rol()`.
> > +local function bitop_rotation(bitop)
> 
> I would rename arg `bitop` to `bitop_func` to highlight the type
> 
> of the value.

Renamed, see the iterational patch below.

> 
> > +  local r = {}
> > +  for i = 1, 4 do
> > +    -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> > +    local int64 = bit.band(i, 7LL)
> > +    r[i] = tonumber(bitop(int64, 32))
> please add comments about magic constants here and below

Added the corresponding comment.

> > +  end
> > +  return r
> > +end
> > +

<snipped>

> > +
> > +test:test('rol', test_64bitness, bitop_rotation, bit.rol)
> > +test:test('ror', test_64bitness, bitop_rotation, bit.ror)
> > +test:test('rshift signed', test_64bitness, bitop_rshift_signed)
> > +test:test('rshift huge',   test_64bitness, bitop_rshift_huge)
> have you added additional whitespaces intentionally?

Yes, I rearranged them a bit to avoid confusion. See the iterative patch
below.

===================================================================
diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
index 6cc0b319..28383bf9 100644
--- a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
+++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
@@ -13,12 +13,15 @@ local bit = require('bit')
 test:plan(4)
 
 -- Generic function for `bit.ror()`, `bit.rol()`.
-local function bitop_rotation(bitop)
+local function bitop_rotation(bitop_func)
   local r = {}
   for i = 1, 4 do
     -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+    -- XXX: Don't use named constants here to match folding rules.
+    -- `7LL` is just some mask, that doesn't change the `i` value.
+    -- `32` is used for the half bit-width rotation.
     local int64 = bit.band(i, 7LL)
-    r[i] = tonumber(bitop(int64, 32))
+    r[i] = tonumber(bitop_func(int64, 32))
   end
   return r
 end
@@ -52,23 +55,23 @@ local function bitop_rshift_huge()
   return r
 end
 
-local function test_64bitness(subtest, payload_func, bitop)
+local function test_64bitness(subtest, payload_func, bitop_func)
   subtest:plan(1)
 
   jit.off()
   jit.flush()
-  local results_joff = payload_func(bitop)
+  local results_joff = payload_func(bitop_func)
   jit.on()
   -- Reset hotcounters.
   jit.opt.start('hotloop=1')
-  local results_jon = payload_func(bitop)
+  local results_jon = payload_func(bitop_func)
   subtest:is_deeply(results_jon, results_joff,
                     'same results for VM and JIT for ' .. subtest.name)
 end
 
-test:test('rol', test_64bitness, bitop_rotation, bit.rol)
-test:test('ror', test_64bitness, bitop_rotation, bit.ror)
 test:test('rshift signed', test_64bitness, bitop_rshift_signed)
 test:test('rshift huge',   test_64bitness, bitop_rshift_huge)
+test:test('rol',           test_64bitness, bitop_rotation, bit.rol)
+test:test('ror',           test_64bitness, bitop_rotation, bit.ror)
 
 test:done(true)
===================================================================

> > +
> > +test:done(true)

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
  2024-10-08 14:24     ` Sergey Kaplun via Tarantool-patches
@ 2024-10-09 14:29       ` Sergey Bronnikov via Tarantool-patches
  2024-10-11 19:12       ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-09 14:29 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

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

Hi, Sergey,

thanks for the fixes! LGTM

On 08.10.2024 17:24, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
>
> On 08.10.24, Sergey Bronnikov wrote:
>
<snipped>

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
  2024-10-08 10:12   ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-11 19:08   ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 10+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-10-11 19:08 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the patch!
LGTM

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
  2024-10-08 14:24     ` Sergey Kaplun via Tarantool-patches
  2024-10-09 14:29       ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-11 19:12       ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 10+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-10-11 19:12 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Hi, Sergey!
Thanks for the fixes!
LGTM

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library
  2024-10-02  8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
  2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
@ 2024-10-18 15:17 ` Sergey Kaplun via Tarantool-patches
  2 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-18 15:17 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches

I've applied the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.2 [2]
and release/2.11 [3].

[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714

-- 
Best regards,
Sergey Kaplun

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-10-18 15:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-02  8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
2024-10-08 10:12   ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:08   ` Maxim Kokryashkin via Tarantool-patches
2024-10-02  8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
2024-10-08 12:07   ` Sergey Bronnikov via Tarantool-patches
2024-10-08 14:24     ` Sergey Kaplun via Tarantool-patches
2024-10-09 14:29       ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:12       ` Maxim Kokryashkin via Tarantool-patches
2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox