Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit] Fix recording of BC_VARG.
Date: Fri,  7 Feb 2025 16:05:00 +0300	[thread overview]
Message-ID: <20250207130500.10406-1-skaplun@tarantool.org> (raw)

From: Mike Pall <mike>

Reported by Bachir Bendrissou.

(cherry picked from commit 62e362afbb1d100c892d2782c5862ad18bc464f2)

When the trace is started after the stitching, it may not have some
slots from the first one. That slot may be the first slot given to the
`select()` function (if it is determined by the call that caused the
stitch). Before the patch, there is no loading of this slot in the
`rec_varg()` for the trace, and the 0 value from slots is taken instead.
Hence, the following recording of `BC_VARG` is incorrect.

This patch fixes this by using the `getslot()` instead of taking the
value directly.

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

Part of tarantool/tarantool#11055
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/fix-recording-bc-varg-used-in-select
Related issues: https://github.com/tarantool/tarantool/issues/11055
THread in ML: https://www.freelists.org/post/luajit/Possible-issue-during-register-allocation-ra-alloc1

 src/lj_record.c                               |  2 +-
 ...-recording-bc-varg-used-in-select.test.lua | 36 +++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/fix-recording-bc-varg-used-in-select.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index 7181b72a..5345fa63 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -1869,7 +1869,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
 	J->maxslot = dst + (BCReg)nresults;
       }
     } else if (select_detect(J)) {  /* y = select(x, ...) */
-      TRef tridx = J->base[dst-1];
+      TRef tridx = getslot(J, dst-1);
       TRef tr = TREF_NIL;
       ptrdiff_t idx = lj_ffrecord_select_mode(J, tridx, &J->L->base[dst-1]);
       if (idx < 0) goto nyivarg;
diff --git a/test/tarantool-tests/fix-recording-bc-varg-used-in-select.test.lua b/test/tarantool-tests/fix-recording-bc-varg-used-in-select.test.lua
new file mode 100644
index 00000000..20b43e07
--- /dev/null
+++ b/test/tarantool-tests/fix-recording-bc-varg-used-in-select.test.lua
@@ -0,0 +1,36 @@
+local tap = require('tap')
+
+-- Test file to demonstrate incorrect recording of `BC_VARG` that
+-- is given to the `select()` function. See also:
+-- https://www.freelists.org/post/luajit/Possible-issue-during-register-allocation-ra-alloc1.
+
+local test = tap.test('fix-recording-bc-varg-used-in-select'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- XXX: Simplify `jit.dump` output.
+local modf = math.modf
+
+local EXPECTED = 'canary'
+local function test_func(...)
+  local first_varg_item
+  for _ = 1, 4 do
+    -- `modf()` is used to create a stitching with a meaningful
+    -- index value, that equals 1, i.e. refers to the first item
+    -- in `...`. The second trace started after stitching does not
+    -- contain the stack slot for the first argument of the
+    -- `select()`. Before the patch, there is no loading of
+    -- this slot for the trace and the 0 value is taken instead.
+    -- Hence, this leads to an incorrect recording of the
+    -- `BC_VARG` with detected `select()`.
+    first_varg_item = select(modf(1, 0), ...)
+  end
+  return first_varg_item
+end
+
+jit.opt.start('hotloop=1')
+test:is(test_func(EXPECTED), EXPECTED, 'corect BC_VARG recording')
+
+test:done(true)
-- 
2.47.1


             reply	other threads:[~2025-02-07 13:05 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-07 13:05 Sergey Kaplun via Tarantool-patches [this message]
2025-02-10 15:14 ` Sergey Bronnikov via Tarantool-patches
2025-02-10 15:18   ` Sergey Kaplun via Tarantool-patches
2025-02-10 17:50     ` Sergey Bronnikov via Tarantool-patches
2025-02-13 10:51 ` Sergey Kaplun 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=20250207130500.10406-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Fix recording of BC_VARG.' \
    /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