[Tarantool-patches] [PATCH luajit 0/3] Narrowing unary minus dualnum

[Tarantool-patches] [PATCH luajit 0/3] Narrowing unary minus dualnum

[Sergey Kaplun via Tarantool-patches]

This patch-set backports the fixes for the unary minus in the DUALNUM
build for 0 and -0. The first commit is required for simplified skip
conditions in the tests.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1418-1422-dualnum-narrowing-0
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1418
* https://github.com/LuaJIT/LuaJIT/issues/1422
* https://github.com/LuaJIT/LuaJIT/issues/1442
* https://github.com/tarantool/tarantool/issues/12134

Mike Pall (3):
  Add ffi.abi("dualnum").
  DUALNUM: Fix narrowing of unary minus.
  DUALNUM: Improve/fix edge cases of unary minus.

 doc/ext_ffi_api.html                          |  2 +
 src/lib_ffi.c                                 |  3 ++
 src/lj_opt_narrow.c                           |  5 +-
 src/vm_arm.dasc                               |  5 +-
 src/vm_arm64.dasc                             |  2 +
 src/vm_mips.dasc                              |  6 ++-
 src/vm_mips64.dasc                            |  6 ++-
 src/vm_ppc.dasc                               |  7 ++-
 src/vm_x64.dasc                               |  4 ++
 src/vm_x86.dasc                               |  5 ++
 .../fix-jit-dump-ir-conv.test.lua             |  4 +-
 .../lj-1166-error-stitch-oom-ir-buff.test.lua |  3 +-
 ...lj-1418-dualnum-narrowing-minus-0.test.lua | 49 +++++++++++++++++
 .../lj-1422-incorrect-unm-for-mzero.test.lua  | 54 +++++++++++++++++++
 .../tarantool-tests/lj-1422-unm-zero.test.lua | 12 +++++
 .../lj-859-math-ceil-sign.test.lua            |  3 +-
 16 files changed, 160 insertions(+), 10 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
 create mode 100644 test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
 create mode 100644 test/tarantool-tests/lj-1422-unm-zero.test.lua

-- 
2.53.0

[Tarantool-patches] [PATCH luajit 1/3] Add ffi.abi("dualnum").

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit a553b3de243b1ae07bdb21da4bdab77148793f76)

This call returns `true` if the LuaJIT is built in DUALNUM mode, `false`
otherwise. It may be useful for testing, trace semantics verification
etc.

All tests where skipconds are relied on the DUALNUM build are updated
with the use of the `ffi.abi()` instead of previous implementations of
checks.

Sergey Kaplun:
* added the description for the feature

Part of tarantool/tarantool#12134
---
 doc/ext_ffi_api.html                                          | 2 ++
 src/lib_ffi.c                                                 | 3 +++
 test/tarantool-tests/fix-jit-dump-ir-conv.test.lua            | 4 +++-
 .../tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua | 3 ++-
 test/tarantool-tests/lj-859-math-ceil-sign.test.lua           | 3 ++-
 5 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/doc/ext_ffi_api.html b/doc/ext_ffi_api.html
index c72191d1..f0a15e7b 100644
--- a/doc/ext_ffi_api.html
+++ b/doc/ext_ffi_api.html
@@ -472,6 +472,8 @@ otherwise. The following parameters are currently defined:
 <td class="abiparam">uwp</td><td class="abidesc">Universal Windows Platform</td></tr>
 <tr class="odd">
 <td class="abiparam">gc64</td><td class="abidesc">64 bit GC references</td></tr>
+<tr class="odd">
+<td class="abiparam">dualnum</td><td class="abidesc">Dual-number mode</td></tr>
 </table>
 
 <h3 id="ffi_os"><tt>ffi.os</tt></h3>
diff --git a/src/lib_ffi.c b/src/lib_ffi.c
index 7988dab8..ce4813da 100644
--- a/src/lib_ffi.c
+++ b/src/lib_ffi.c
@@ -755,6 +755,9 @@ LJLIB_CF(ffi_abi)	LJLIB_REC(.)
 #endif
 #if LJ_GC64
     "\004gc64"
+#endif
+#if LJ_DUALNUM
+    "\007dualnum"
 #endif
   ) >= 0;
   setboolV(L->top-1, b);
diff --git a/test/tarantool-tests/fix-jit-dump-ir-conv.test.lua b/test/tarantool-tests/fix-jit-dump-ir-conv.test.lua
index a2ab8efc..444a3a05 100644
--- a/test/tarantool-tests/fix-jit-dump-ir-conv.test.lua
+++ b/test/tarantool-tests/fix-jit-dump-ir-conv.test.lua
@@ -4,6 +4,8 @@ local test = tap.test('fix-jit-dump-ir-conv'):skipcond({
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
 
+local ffi = require('ffi')
+
 test:plan(2)
 
 -- Test file to demonstrate LuaJIT incorrect `jit.dump()` output
@@ -33,7 +35,7 @@ local traces = jparse.finish()
 
 -- Skip tests for DUALNUM mode since it has no conversions (for
 -- the same cases).
-local IS_DUALNUM = not traces[1]:has_ir('num SLOAD')
+local IS_DUALNUM = ffi.abi('dualnum')
 
 test:ok(IS_DUALNUM or traces[1]:has_ir('CONV.*int.num index'),
         'correct dump for index')
diff --git a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
index dc21cfbf..00edafbd 100644
--- a/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
+++ b/test/tarantool-tests/lj-1166-error-stitch-oom-ir-buff.test.lua
@@ -10,10 +10,11 @@ local test = tap.test('lj-1166-error-stitch-oom-ir-buff'):skipcond({
   ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
 })
 
+local ffi = require('ffi')
 local jparse = require('utils').jit.parse
 local allocinject = require('allocinject')
 
-local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)
+local IS_DUALNUM = ffi.abi('dualnum')
 
 -- XXX: Avoid other traces compilation due to hotcount collisions
 -- for predictable results.
diff --git a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua
index a67de888..6675b5cb 100644
--- a/test/tarantool-tests/lj-859-math-ceil-sign.test.lua
+++ b/test/tarantool-tests/lj-859-math-ceil-sign.test.lua
@@ -1,4 +1,5 @@
 local tap = require('tap')
+local ffi = require('ffi')
 
 -- Test file to demonstrate the incorrect LuaJIT's behaviour
 -- for `math.ceil(x)` when argument `x`: -1 < x < -0.5.
@@ -8,7 +9,7 @@ local test = tap.test('lj-859-math-ceil-sign')
 
 test:plan(1)
 
-local IS_DUALNUM = tostring(tonumber('-0')) ~= tostring(-0)
+local IS_DUALNUM = ffi.abi('dualnum')
 local IS_X86_64 = jit.arch == 'x86' or jit.arch == 'x64'
 
 -- Use `tostring()` to compare the sign of the returned value.
-- 
2.53.0

[Tarantool-patches] [PATCH luajit 2/3] DUALNUM: Fix narrowing of unary minus.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry picked from commit b1cd2f83b5d085bb71368b87c91a461be77d4364)

`lj_opt_narrow_unm()` in the DUALNUM mode narrows doubles too
optimistic, missing 0 check. In that case, the narrowing of 0 is
incorrect. This leads to the assertion failure in `rec_check_slots()`
for the string obtained from the corresponding number.

This patch fixes it by restricting the check of the given TValue.

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

Part of tarantool/tarantool#12134
---
 src/lj_opt_narrow.c                           |  4 +-
 ...lj-1418-dualnum-narrowing-minus-0.test.lua | 49 +++++++++++++++++++
 2 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua

diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
index 6b6f20d3..6e3e9533 100644
--- a/src/lj_opt_narrow.c
+++ b/src/lj_opt_narrow.c
@@ -553,9 +553,9 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
   rc = conv_str_tonum(J, rc, vc);
   if (tref_isinteger(rc)) {
     uint32_t k = (uint32_t)numberVint(vc);
-    if ((LJ_DUALNUM || k != 0) && k != 0x80000000u) {
+    if ((tvisint(vc) || k != 0) && k != 0x80000000u) {
       TRef zero = lj_ir_kint(J, 0);
-      if (!LJ_DUALNUM)
+      if (!tvisint(vc))
 	emitir(IRTGI(IR_NE), rc, zero);
       return emitir(IRTGI(IR_SUBOV), zero, rc);
     }
diff --git a/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
new file mode 100644
index 00000000..84f17953
--- /dev/null
+++ b/test/tarantool-tests/lj-1418-dualnum-narrowing-minus-0.test.lua
@@ -0,0 +1,49 @@
+local tap = require('tap')
+
+-- This test demonstrates LuaJIT's incorrect narrowing
+-- optimization in the DUALNUM mode for 0.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1418.
+
+local test = tap.test('lj-1418-dualnum-narrowing-minus-0'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+local tostring = tostring
+
+local function test_const_on_trace(x)
+  local zero = x % 1
+  local mzero = -zero
+  -- Bad IR slot with enabled optimizations.
+  local res = tostring(mzero)
+  return res
+end
+
+local function test_non_const_on_trace(a, b)
+  local mb_zero = a % b
+  -- Too optimistic optimization without check for the 0 corner
+  -- case.
+  local mb_mzero = -mb_zero
+  local res = tostring(mb_mzero)
+  return res
+end
+
+jit.opt.start('hotloop=1')
+
+-- Hot trace.
+test_const_on_trace(1)
+-- Compile trace.
+test:is(test_const_on_trace(1), '-0', 'correct const value on trace')
+
+-- Reset hotcounts.
+jit.opt.start('hotloop=1')
+
+-- Hot trace.
+test_non_const_on_trace(2, 3)
+-- Record trace, use non zero result value to record.
+test_non_const_on_trace(2, 3)
+-- Misbehaviour on trace with result zero value.
+test:is(test_non_const_on_trace(2, 1), '-0', 'correct non-const value on trace')
+
+test:done(true)
-- 
2.53.0

[Tarantool-patches] [PATCH luajit 3/3] DUALNUM: Improve/fix edge cases of unary minus.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Sergey Kaplun.

(cherry picked from commit 707c12bf00dafdfd3899b1a6c36435dbbf6c7022)

In a single-number build (default for x86/x64), `-(0)` is always `-0`
due to its number type. However, in DUALNUM mode, the `0` may have the
integer type, and for this integer, `BC_UNM` has no effect. `0` may be
represented via number or integer type, where the resulting `0` is
constructed during initial parsing (`lj_strscan_scan()`) or arithmetic
operations (`BC_MODVV`). This inconsistency may break the JIT semantics
and lead to the assertion failure in the `lj_check_slots()`.

This patch fixes the incorrect `BC_UNM` for integer operand 0. Also, the
narrowing optimization for the unary minus should be more strict to
avoid incorrect narrowing for integer slots and make JIT intact with the
VM semantics.

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

Part of tarantool/tarantool#12134
---
 src/lj_opt_narrow.c                           |  5 +-
 src/vm_arm.dasc                               |  5 +-
 src/vm_arm64.dasc                             |  2 +
 src/vm_mips.dasc                              |  6 ++-
 src/vm_mips64.dasc                            |  6 ++-
 src/vm_ppc.dasc                               |  7 ++-
 src/vm_x64.dasc                               |  4 ++
 src/vm_x86.dasc                               |  5 ++
 .../lj-1422-incorrect-unm-for-mzero.test.lua  | 54 +++++++++++++++++++
 .../tarantool-tests/lj-1422-unm-zero.test.lua | 12 +++++
 10 files changed, 99 insertions(+), 7 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
 create mode 100644 test/tarantool-tests/lj-1422-unm-zero.test.lua

diff --git a/src/lj_opt_narrow.c b/src/lj_opt_narrow.c
index 6e3e9533..3bf7320d 100644
--- a/src/lj_opt_narrow.c
+++ b/src/lj_opt_narrow.c
@@ -553,10 +553,9 @@ TRef lj_opt_narrow_unm(jit_State *J, TRef rc, TValue *vc)
   rc = conv_str_tonum(J, rc, vc);
   if (tref_isinteger(rc)) {
     uint32_t k = (uint32_t)numberVint(vc);
-    if ((tvisint(vc) || k != 0) && k != 0x80000000u) {
+    if (k != 0 && k != 0x80000000u) {
       TRef zero = lj_ir_kint(J, 0);
-      if (!tvisint(vc))
-	emitir(IRTGI(IR_NE), rc, zero);
+      emitir(IRTGI(IR_NE), rc, zero);
       return emitir(IRTGI(IR_SUBOV), zero, rc);
     }
     rc = emitir(IRTN(IR_CONV), rc, IRCONV_NUM_INT);
diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
index 7095e660..785d13ac 100644
--- a/src/vm_arm.dasc
+++ b/src/vm_arm.dasc
@@ -2930,13 +2930,16 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |  bhi ->vmeta_unm
     |  eorne CARG2, CARG2, #0x80000000
     |  bne >5
-    |  rsbseq CARG1, CARG1, #0
+    |  rsbs CARG1, CARG1, #0
+    |  ldrdeq CARG12, >8
     |  ldrdvs CARG12, >9
     |5:
     |  strd CARG12, [BASE, RA]
     |   ins_next3
     |
     |.align 8
+    |8:
+    |  .long 0x00000000, 0x80000000	// -0.
     |9:
     |  .long 0x00000000, 0x41e00000	// 2^31.
     break;
diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 6600e226..c64b736b 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -2481,6 +2481,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |   movz CARG3, #0x41e0, lsl #48	// 2^31.
     |   add TMP0, TMP0, TISNUM
     |  csel TMP0, TMP0, CARG3, vc
+    |   movz CARG3, #0x8000, lsl #48	// -0.
+    |  csel TMP0, TMP0, CARG3, ne
     |5:
     |  str TMP0, [BASE, RA, lsl #3]
     |  ins_next
diff --git a/src/vm_mips.dasc b/src/vm_mips.dasc
index 32caabf7..dcb04ed8 100644
--- a/src/vm_mips.dasc
+++ b/src/vm_mips.dasc
@@ -3319,7 +3319,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |   addu RA, BASE, RA
     |  bne SFARG1HI, TISNUM, >2
     |.  lw SFARG1LO, LO(RB)
-    |  lui TMP1, 0x8000
+    |  beqz SFARG1LO, >3
+    |.  lui TMP1, 0x8000
     |  beq SFARG1LO, TMP1, ->vmeta_unm	// Meta handler deals with -2^31.
     |.  negu SFARG1LO, SFARG1LO
     |1:
@@ -3333,6 +3334,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |.  lui TMP1, 0x8000
     |  b <1
     |.  xor SFARG1HI, SFARG1HI, TMP1
+    |3:
+    |  b <1
+    |.  lui SFARG1HI, 0x8000	// -0.
     break;
   case BC_LEN:
     |  // RA = dst*8, RD = src*8
diff --git a/src/vm_mips64.dasc b/src/vm_mips64.dasc
index 44fba36c..34da6473 100644
--- a/src/vm_mips64.dasc
+++ b/src/vm_mips64.dasc
@@ -3563,7 +3563,8 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |  sextw CARG1, CARG1
     |  beq CARG1, TMP1, ->vmeta_unm	// Meta handler deals with -2^31.
     |.  negu CARG1, CARG1
-    |  zextw CARG1, CARG1
+    |  beqz CARG1, >3
+    |.  zextw CARG1, CARG1
     |  settp CARG1, TISNUM
     |1:
     |  ins_next1
@@ -3575,6 +3576,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |.  dsll TMP1, TMP1, 32
     |  b <1
     |.  xor CARG1, CARG1, TMP1
+    |3:
+    |  b <1
+    |.  dsll CARG1, TMP1, 32
     break;
   case BC_LEN:
     |  // RA = dst*8, RD = src*8
diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc
index 980ad897..88fa7b67 100644
--- a/src/vm_ppc.dasc
+++ b/src/vm_ppc.dasc
@@ -3803,11 +3803,13 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |  bne >5
     |.if GPR64
     |  lus TMP2, 0x8000
-    |  neg TMP0, TMP0
+    |  neg. TMP0, TMP0
+    |  beq >8
     |  cmplw TMP0, TMP2
     |  beq >4
     |.else
     |  nego. TMP0, TMP0
+    |  beq >8
     |  bso >4
     |1:
     |.endif
@@ -3834,6 +3836,9 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |   stw TMP0, 4(RA)
     |.if DUALNUM
     |  b <3
+    |8:
+    |  lus TMP1, 0x8000			// -0.
+    |  b <7
     |.else
     |  ins_next2
     |.endif
diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 8b6781a6..ee669887 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -3179,11 +3179,15 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |.if DUALNUM
     |  checkint RB, >5
     |  neg RBd
+    |  jz >3
     |  jo >4
     |  setint RB
     |9:
     |  mov [BASE+RA*8], RB
     |  ins_next
+    |3:
+    |  mov64 RB, U64x(80000000,00000000)  // -0.
+    |  jmp <9
     |4:
     |  mov64 RB, U64x(41e00000,00000000)  // 2^31.
     |  jmp <9
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index 7c11c78e..027bdcd6 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -3780,11 +3780,16 @@ static void build_ins(BuildCtx *ctx, BCOp op, int defop)
     |  checkint RD, >5
     |  mov RB, [BASE+RD*8]
     |  neg RB
+    |  jz >3
     |  jo >4
     |  mov dword [BASE+RA*8+4], LJ_TISNUM
+    |8:
     |  mov dword [BASE+RA*8], RB
     |9:
     |  ins_next
+    |3:
+    |  mov dword [BASE+RA*8+4], 0x80000000  // -0.
+    |  jmp <8
     |4:
     |  mov dword [BASE+RA*8+4], 0x41e00000  // 2^31.
     |  mov dword [BASE+RA*8], 0
diff --git a/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua b/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
new file mode 100644
index 00000000..7046784c
--- /dev/null
+++ b/test/tarantool-tests/lj-1422-incorrect-unm-for-mzero.test.lua
@@ -0,0 +1,54 @@
+local tap = require('tap')
+
+-- This test demonstrates LuaJIT's inconsistencies in the VM and
+-- the JIT engine in the DUALNUM mode for 0.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1422.
+
+local test = tap.test('lj-1422-incorrect-unm-for-mzero'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(3)
+
+local tonumber = tonumber
+local tostring = tostring
+
+local function always_number(val)
+  return tonumber(val) or 1
+end
+
+-- Yielded int type for non-x86 arches.
+local function modvn(v1)
+  return always_number(v1) % 1
+end
+
+local function unm(v)
+  return -v
+end
+
+jit.opt.start('hotloop=1', 'hotexit=1')
+
+always_number(nil) -- Root trace.
+always_number(nil)
+
+local stack_slot = nil
+for _ = 1, 5 do
+  always_number(0) -- Compile side trace.
+  -- The side trace crashes in the `rec_check_slots()` for non-x86
+  -- arches before the patch.
+  stack_slot = tostring(unm(modvn(stack_slot)))
+end
+
+test:is(stack_slot, '-0', 'correct result of the trace execution')
+
+-- `tonumber()` recording and conversion to number.
+local results = {nil, nil, nil, nil}
+for i = 1, 4 do
+  local slot = -tonumber('0')
+  results[i] = tostring(slot)
+end
+
+test:is(results[1], '-0', 'correct result of the expression')
+test:samevalues(results, 'correct result of the tonumber recording')
+
+test:done(true)
diff --git a/test/tarantool-tests/lj-1422-unm-zero.test.lua b/test/tarantool-tests/lj-1422-unm-zero.test.lua
new file mode 100644
index 00000000..77062418
--- /dev/null
+++ b/test/tarantool-tests/lj-1422-unm-zero.test.lua
@@ -0,0 +1,12 @@
+local tap = require('tap')
+
+-- The test file to demonstrate LuaJIT's incorrect BC_UNM for the
+-- 0 operand for the DUALNUM mode.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/1422.
+
+local test = tap.test('lj-1422-unm-zero')
+test:plan(1)
+
+test:ok(tostring(-0) == '-0', 'correct unary minus for 0')
+
+test:done(true)
-- 
2.53.0