Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maksim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, sergos@tarantool.org,
	imun@tarantool.org, skaplun@tarantool.org,
	m.kokryashkin@tarantool.org
Subject: [Tarantool-patches] [PATCH luajit v4 4/4] Fix IR_RENAME snapshot number. Follow-up fix for a32aeadc.
Date: Mon, 15 May 2023 12:16:22 +0300	[thread overview]
Message-ID: <20230515091622.30232-5-max.kokryashkin@gmail.com> (raw)
In-Reply-To: <20230515091622.30232-1-max.kokryashkin@gmail.com>

From: Mike Pall <mike>

Reported by Victor Bombi, analyzed by XmiliaH. Thanks!

(cherry-picked from commit bf51d3535109c4745bfbbe19a5587a9eac00259a)

If the `snapalloc` flag is set, then the allocation has not
occurred yet, meaning that rename is applied to the next
snapshot. Otherwise, refs are already allocated and rename
is applied to the current snapshot.

Maxim Kokryashkin:
* added the description and the test for the problem

Part of tarantool/tarantool#7745
Part of tarantool/tarantool#8069
---
 src/lj_asm.c                                  |  9 ++-
 .../lj-688-snap-ir-rename.test.lua            | 60 +++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-688-snap-ir-rename.test.lua

diff --git a/src/lj_asm.c b/src/lj_asm.c
index f7c40fea..9bbfb2bf 100644
--- a/src/lj_asm.c
+++ b/src/lj_asm.c
@@ -682,7 +682,14 @@ static void ra_rename(ASMState *as, Reg down, Reg up)
   RA_DBGX((as, "rename    $f $r $r", regcost_ref(as->cost[up]), down, up));
   emit_movrr(as, ir, down, up);  /* Backwards codegen needs inverse move. */
   if (!ra_hasspill(IR(ref)->s)) {  /* Add the rename to the IR. */
-    ra_addrename(as, down, ref, as->snapno);
+    /*
+    ** The rename is effective at the subsequent (already emitted) exit
+    ** branch. This is for the current snapshot (as->snapno). Except if we
+    ** haven't yet allocated any refs for the snapshot (as->snapalloc == 1),
+    ** then it belongs to the next snapshot.
+    ** See also the discussion at asm_snap_checkrename().
+    */
+    ra_addrename(as, down, ref, as->snapno + as->snapalloc);
   }
 }
 
diff --git a/test/tarantool-tests/lj-688-snap-ir-rename.test.lua b/test/tarantool-tests/lj-688-snap-ir-rename.test.lua
new file mode 100644
index 00000000..f626cfb6
--- /dev/null
+++ b/test/tarantool-tests/lj-688-snap-ir-rename.test.lua
@@ -0,0 +1,60 @@
+local tap = require('tap')
+local test = tap.test('lj-688-snap-ir-rename'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+test:plan(1)
+
+jit.opt.start('hotloop=1')
+
+-- IR for the loop below looks like the following
+-- before the patch:
+--
+-- 0021 ------------ LOOP ------------
+-- ....              SNAP   #6   [ ---- ---- 0019 false 0007 ]
+-- 0022       >  num NE     0019  +1.1
+-- ....              SNAP   #7   [ ---- ---- 0019 false 0007 ]
+-- 0023 rbp   >  int CONV   0019  int.num
+-- 0024       >  int ABC    0013  0023
+-- 0025          p32 AREF   0015  0023
+-- 0026 xmm6  >  num ALOAD  0025
+-- ....              SNAP   #8   [ ---- ---- 0019 false ---- ]
+-- 0027       >  num ULE    0026  +0
+-- 0028 xmm7   + num ADD    0019  +1
+-- ....              SNAP   #9   [ ---- ---- 0019 false ]
+-- 0029       >  num LT     0028  +5
+-- 0030 xmm7     num PHI    0019  0028
+-- 0031 xmm6     nil RENAME 0019  #8
+-- ---- TRACE 1 stop -> loop
+--
+-- RENAME instruction is applied to the 8th snapshot, when it
+-- should be applied to the 9th. After the patch that line looks
+-- the following way:
+--
+-- 0031 xmm6     nil RENAME 0019  #9
+
+-- XXX: Note, that reproducer below won't fail on ARM/ARM64
+-- even before the patch, because `IR_RENAME` is not emitted
+-- for any of the instructions produced.
+-- Although it is possible to achieve the same faulty behavior
+-- before the patch on ARM/ARM64, it requires a more complex
+-- reproducer. Since the code affected by the patch is
+-- platform-agnostic, there is no real necessity to test it
+-- against ARM/ARM64 separately.
+--
+-- See also https://drive.google.com/file/d/1iYkFx3F0DOtB9o9ykWfCEm-OdlJNCCL0/view?usp=share_link
+
+
+local vals = {-0.1, 0.1, -0.1, 0.1}
+local i = 1
+local _
+while i < 5 do
+    assert(i ~= 1.1)
+    local l1 = vals[i]
+    _ = l1 > 0
+    i = i + 1
+end
+
+test:ok(true, 'IR_RENAME is fine')
+-- `test:check() and 0 or 1` is replaced with just test:check()
+-- here, because otherwise, it affects the renaming process.
+os.exit(test:check())
-- 
2.39.2 (Apple Git-143)


  parent reply	other threads:[~2023-05-15  9:18 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-15  9:16 [Tarantool-patches] [PATCH luajit v4 0/4] jit: add exception unwinding Maksim Kokryashkin via Tarantool-patches
2023-05-15  9:16 ` [Tarantool-patches] [PATCH luajit v4 1/4] Handle on-trace OOM errors from helper functions Maksim Kokryashkin via Tarantool-patches
2023-05-24  6:09   ` Igor Munkin via Tarantool-patches
2023-05-15  9:16 ` [Tarantool-patches] [PATCH luajit v4 2/4] Disable unreliable assertion for external frame unwinding Maksim Kokryashkin via Tarantool-patches
2023-05-24  6:09   ` Igor Munkin via Tarantool-patches
2023-05-15  9:16 ` [Tarantool-patches] [PATCH luajit v4 3/4] OSX: " Maksim Kokryashkin via Tarantool-patches
2023-05-24  6:10   ` Igor Munkin via Tarantool-patches
2023-05-15  9:16 ` Maksim Kokryashkin via Tarantool-patches [this message]
2023-05-24  6:10   ` [Tarantool-patches] [PATCH luajit v4 4/4] Fix IR_RENAME snapshot number. Follow-up fix for a32aeadc Igor Munkin via Tarantool-patches
2023-05-22  9:27 ` [Tarantool-patches] [PATCH luajit v4 0/4] jit: add exception unwinding Sergey Kaplun via Tarantool-patches
2023-05-25  6:17 ` 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20230515091622.30232-5-max.kokryashkin@gmail.com \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergos@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit v4 4/4] Fix IR_RENAME snapshot number. Follow-up fix for a32aeadc.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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