<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hello, Sergey,</p>
<p>thanks for the patch! Please see my comments below.<br>
</p>
<div class="moz-cite-prefix">On 02.10.2024 11:09, Sergey Kaplun
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7531b1a6a3f39f8f2d83a54befdc67af987cebaf.1727855711.git.skaplun@tarantool.org">
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
typo: is >= 2,147,483,647<br>
<blockquote type="cite"
cite="mid:7531b1a6a3f39f8f2d83a54befdc67af987cebaf.1727855711.git.skaplun@tarantool.org">
<pre class="moz-quote-pre" wrap="">
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, <a class="moz-txt-link-freetext" href="https://github.com/LuaJIT/LuaJIT/issues/1079">https://github.com/LuaJIT/LuaJIT/issues/1079</a>.
+
+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)</pre>
</blockquote>
<p>I would rename arg `bitop` to `bitop_func` to highlight the type<br>
</p>
<p>of the value.<br>
</p>
<blockquote type="cite"
cite="mid:7531b1a6a3f39f8f2d83a54befdc67af987cebaf.1727855711.git.skaplun@tarantool.org">
<pre class="moz-quote-pre" wrap="">
+ 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))</pre>
</blockquote>
please add comments about magic constants here and below<br>
<blockquote type="cite"
cite="mid:7531b1a6a3f39f8f2d83a54befdc67af987cebaf.1727855711.git.skaplun@tarantool.org">
<pre class="moz-quote-pre" wrap="">
+ 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]: <a class="moz-txt-link-freetext" href="https://bitop.luajit.org/semantics.html">https://bitop.luajit.org/semantics.html</a>
+ 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)</pre>
</blockquote>
have you added additional whitespaces intentionally?<br>
<blockquote type="cite"
cite="mid:7531b1a6a3f39f8f2d83a54befdc67af987cebaf.1727855711.git.skaplun@tarantool.org">
<pre class="moz-quote-pre" wrap="">
+
+test:done(true)
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>