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

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

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