Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>,
	Sergey Ostanevich <sergos@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit] Fix bytecode register allocation for comparisons.
Date: Mon, 19 Jul 2021 10:36:32 +0300	[thread overview]
Message-ID: <20210719073632.12008-1-skaplun@tarantool.org> (raw)

From: Mike Pall <mike>

(cherry picked from commit 2f3f07882fb4ad9c64967d7088461b1ca0a25d3a)

When LuaJIT is build with LJ_FR2 (GC64), information about frame takes
two slots -- the first takes the TValue with the function to call, the
second takes the additional frame information. The recording JIT
machinery works 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. It is done either by the
return values either this slot is cleared (set to zero) manually with
the next bytecode with RA dst mode with the assumption, that the dst RA
takes the next slot after TREF_FRAME, i.e. an earlier instruction uses
the smallest possible destination register (see `lj_record_ins()` for
the details).

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

This patch fixes bytecode register allocation by changing the 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

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6227-fix-bytecode-allocator-for-comp
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6227-fix-bytecode-allocator-for-comp
Issue: https://github.com/tarantool/tarantool/issues/6227

 src/lj_parse.c                                |  7 +++-
 ...ytecode-allocator-for-comparisons.test.lua | 41 +++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua

diff --git a/src/lj_parse.c b/src/lj_parse.c
index 08f7cfa6..a6325a76 100644
--- a/src/lj_parse.c
+++ b/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. */
diff --git a/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua
new file mode 100644
index 00000000..66f6885e
--- /dev/null
+++ b/test/tarantool-tests/gh-6227-bytecode-allocator-for-comparisons.test.lua
@@ -0,0 +1,41 @@
+local tap = require('tap')
+local test = tap.test('gh-6227-bytecode-allocator-for-comparisons')
+-- 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.
+-- Also set `J->maxslot` to zero.
+-- The upvalue function to call is loaded to 0 slot.
+local function bump_frame()
+  -- First call function with RET0 to set TREF_FRAME in the
+  -- last slot.
+  empty()
+  -- Test ISGE or ISGT bytecode. These bytecodes swap their
+  -- operands. Also, a constant is always loaded into the slot
+  -- smaller than upvalue. So, if upvalue loads before KSHORT,
+  -- then the difference between registers is more than 2 (2 is
+  -- needed for LJ_FR2) and TREF_FRAME slot is not rewriting by
+  -- the bytecode after call and return as expected. That leads
+  -- to recording slots inconsistency and assertion failure at
+  -- `rec_check_slots()`.
+  empty(1>uv)
+for _ = 1,3 do
+  bump_frame()
+os.exit(test:check() and 0 or 1)

             reply	other threads:[~2021-07-19  7:37 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-19  7:36 Sergey Kaplun via Tarantool-patches [this message]
2021-08-01 10:43 ` Igor Munkin via Tarantool-patches
2021-08-01 17:10   ` Sergey Kaplun via Tarantool-patches
2021-08-16  7:20     ` Igor Munkin via Tarantool-patches
2021-08-16 16:40       ` Sergey Kaplun via Tarantool-patches
2021-08-16 16:27         ` Igor Munkin via Tarantool-patches
2021-08-17  7:36           ` Vitaliia Ioffe via Tarantool-patches
2021-08-10 17:03 ` Sergey Ostanevich via Tarantool-patches
2021-08-16 16:44   ` Sergey Kaplun via Tarantool-patches
2021-08-17  9:24 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210719073632.12008-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Fix bytecode register allocation for comparisons.' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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