Fix bytecode register allocation for comparisons.

(cherry picked from commit 2f3f078)

When LuaJIT is built with LJ_FR2 (e.g. with GC64 mode enabled),
information about frame takes two slots -- the first takes the TValue
with the function to be called, the second takes the framelink. The JIT
recording machinery does pretty the same -- the function IR_KGC is
loaded in the first slot, and the second is set to TREF_FRAME value.
This value should be rewritten after return from a callee. This slot is
cleared either by return values or manually (set to zero), when there
are no values to return. The latter case is done by the next bytecode
with RA dst mode. This obliges that the destination of RA takes the next
slot after TREF_FRAME. Hence, an earlier instruction must use the
smallest possible destination register (see `lj_record_ins()` for the
details).

Bytecode emitter swaps operands for ISGT and ISGE comparisons. As a
result, the aforementioned rule for registers allocations may be
violated. When it happens for a chunk being recorded, the slot with
TREF_FRAME is not rewritten (but the next empty slot after TREF_FRAME
is). This leads to JIT slots inconsistency and assertion failure in
`rec_check_slots()` during recording of the next bytecode instruction.

This patch fixes bytecode register allocation by changing the VM
register allocation order in case of ISGT and ISGE bytecodes.

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

Resolves tarantool/tarantool#6227
Part of tarantool/tarantool#5629

Reviewed-by: Sergey Ostanevich <[email protected]>
Reviewed-by: Igor Munkin <[email protected]>
Signed-off-by: Igor Munkin <[email protected]>

Author: Mike Pall

src/lj_parse.c

@@ -853,9 +853,12 @@ static void bcemit_comp(FuncState *fs, BinOpr opr, ExpDesc *e1, ExpDesc *e2)
       e1 = e2; e2 = eret;  /* Swap operands. */
       op = ((op-BC_ISLT)^3)+BC_ISLT;
       expr_toval(fs, e1);
+      ra = expr_toanyreg(fs, e1);
+      rd = expr_toanyreg(fs, e2);
+    } else {
+      rd = expr_toanyreg(fs, e2);
+      ra = expr_toanyreg(fs, e1);
     }
-    rd = expr_toanyreg(fs, e2);
-    ra = expr_toanyreg(fs, e1);
     ins = BCINS_AD(op, ra, rd);
   }
   /* Using expr_free might cause asserts if the order is wrong. */

test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua

@@ -0,0 +1,54 @@
+local tap = require('tap')
+local test = tap.test('gh-6227-bytecode-allocator-for-comparisons')
+test:plan(1)
+
+-- Test file to demonstrate assertion failure during recording
+-- wrong allocated bytecode for comparisons.
+-- See also https://github.com/tarantool/tarantool/issues/6227.
+
+-- Need function with RET0 bytecode to avoid reset of
+-- the first JIT slot with frame info. Also need no assignments
+-- by the caller.
+local function empty() end
+
+local uv = 0
+
+-- This function needs to reset register enumerating.
+-- `J->maxslot` is initialized with `nargs` (i.e. zero in this
+-- case) in `rec_call_setup()`.
+local function bump_frame()
+  -- First call function with RET0 to set TREF_FRAME in the
+  -- last slot.
+  empty()
+  -- The old bytecode to be recorded looks like the following:
+  -- 0000  . FUNCF    4
+  -- 0001  . UGET     0   0      ; empty
+  -- 0002  . CALL     0   1   1
+  -- 0000  . . JFUNCF   1   1
+  -- 0001  . . RET0     0   1
+  -- 0002  . CALL     0   1   1
+  -- 0003  . UGET     0   0      ; empty
+  -- 0004  . UGET     3   1      ; uv
+  -- 0005  . KSHORT   2   1
+  -- 0006  . ISLT     3   2
+  -- Test ISGE or ISGT bytecode. These bytecodes swap their
+  -- operands (consider ISLT above).
+  -- Two calls of `empty()` function in a row is necessary for 2
+  -- slot gap in LJ_FR2 mode.
+  -- Upvalue loads before KSHORT, so the difference between slot
+  -- for upvalue `empty` (function to be called) and slot for
+  -- upvalue `uv` is more than 2. Hence, TREF_FRAME slot is not
+  -- rewritten by the bytecode after return from `empty()`
+  -- function as expected. That leads to recording slots
+  -- inconsistency and assertion failure at `rec_check_slots()`.
+  empty(1>uv)
+end
+
+jit.opt.start('hotloop=1')
+
+for _ = 1, 3 do
+  bump_frame()
+end
+
+test:ok(true)
+os.exit(test:check() and 0 or 1)