Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Maxim Kokryashkin <m.kokryashkin@tarantool.org>,
	Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH luajit] Consider slots used by upvalues in use-def analysis.
Date: Wed,  7 Feb 2024 17:51:08 +0300	[thread overview]
Message-ID: <20240207145108.29786-1-skaplun@tarantool.org> (raw)

From: Mike Pall <mike>

Reported by XmiliaH.

(cherry picked from commit 3a654999c6f00de4cb9e61232d23579442e544a0)

`snap_usedef()` analysis doesn't check slots for child upvalues of the
currentlly recorded function in use-def analysis. Hence, such slots may
be considered unused and not stored in the snapshot. So on snapshot
restoration, values from these slots may be lost.

This patch adds a marking for all such local upvalues.

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

Part of tarantool/tarantool#9595
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-737-snap-usedef-upvalues
Tarantool PR: https://github.com/tarantool/tarantool/pull/9662
Related issues:
* https://github.com/tarantool/tarantool/issues/9595
* https://github.com/LuaJIT/LuaJIT/issues/737

 src/lj_snap.c                                 | 35 +++++++++++-
 .../lj-737-snap-use-def-upvalues.test.lua     | 55 +++++++++++++++++++
 2 files changed, 87 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-737-snap-use-def-upvalues.test.lua

diff --git a/src/lj_snap.c b/src/lj_snap.c
index 26352080..1ac9296a 100644
--- a/src/lj_snap.c
+++ b/src/lj_snap.c
@@ -302,6 +302,31 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf,
   return 0;  /* unreachable */
 }
 
+/* Mark slots used by upvalues of child prototypes as used. */
+void snap_useuv(GCproto *pt, uint8_t *udf)
+{
+  /* This is a coarse check, because it's difficult to correlate the lifetime
+  ** of slots and closures. But the number of false positives is quite low.
+  ** A false positive may cause a slot not to be purged, which is just
+  ** a missed optimization.
+  */
+  if ((pt->flags & PROTO_CHILD)) {
+    ptrdiff_t i, j, n = pt->sizekgc;
+    GCRef *kr = mref(pt->k, GCRef) - 1;
+    for (i = 0; i < n; i++, kr--) {
+      GCobj *o = gcref(*kr);
+      if (o->gch.gct == ~LJ_TPROTO) {
+	for (j = 0; j < gco2pt(o)->sizeuv; j++) {
+	  uint32_t v = proto_uv(gco2pt(o))[j];
+	  if ((v & PROTO_UV_LOCAL)) {
+	    udf[(v & 0xff)] = 0;
+	  }
+	}
+      }
+    }
+  }
+}
+
 /* Purge dead slots before the next snapshot. */
 void lj_snap_purge(jit_State *J)
 {
@@ -310,9 +335,12 @@ void lj_snap_purge(jit_State *J)
   if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams)
     maxslot = J->pt->numparams;
   s = snap_usedef(J, udf, J->pc, maxslot);
-  for (; s < maxslot; s++)
-    if (udf[s] != 0)
-      J->base[s] = 0;  /* Purge dead slots. */
+  if (s < maxslot) {
+    snap_useuv(J->pt, udf);
+    for (; s < maxslot; s++)
+      if (udf[s] != 0)
+	J->base[s] = 0;  /* Purge dead slots. */
+  }
 }
 
 /* Shrink last snapshot. */
@@ -325,6 +353,7 @@ void lj_snap_shrink(jit_State *J)
   BCReg maxslot = J->maxslot;
   BCReg baseslot = J->baseslot;
   BCReg minslot = snap_usedef(J, udf, snap_pc(&map[nent]), maxslot);
+  if (minslot < maxslot) snap_useuv(J->pt, udf);
   maxslot += baseslot;
   minslot += baseslot;
   snap->nslots = (uint8_t)maxslot;
diff --git a/test/tarantool-tests/lj-737-snap-use-def-upvalues.test.lua b/test/tarantool-tests/lj-737-snap-use-def-upvalues.test.lua
new file mode 100644
index 00000000..8535f9f6
--- /dev/null
+++ b/test/tarantool-tests/lj-737-snap-use-def-upvalues.test.lua
@@ -0,0 +1,55 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour in use-def
+-- snapshot analysis for local upvalues.
+-- See also https://github.com/LuaJIT/LuaJIT/issues/737.
+
+local test = tap.test('lj-737-snap-use-def-upvalues'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- XXX: simplify `jit.dump()` output.
+local fmod = math.fmod
+
+local EXPECTED = 'expected'
+
+jit.opt.start('hotloop=1')
+
+local function wrapped_trace(create_closure)
+  local local_upvalue, closure
+  if create_closure then
+    closure = function() return local_upvalue end
+  end
+  for i = 1, 4 do
+    -- On the second iteration, the trace is recorded.
+    if i == 2 then
+      -- Before the patch, this slot was considered unused by
+      -- use-def analysis in the `snap_usedef()` since there are
+      -- no open unpvalues for `closure()` on recording (1st call).
+      local_upvalue = EXPECTED
+      -- luacheck: ignore
+      -- Emit an additional snapshot after setting the
+      -- upvalue.
+      if i == 0 then end
+      -- Stitching ends the trace here.
+      fmod(1,1)
+      return closure
+    end
+  end
+end
+
+-- Compile the trace.
+local func_with_uv = wrapped_trace(false)
+assert(func_with_uv == nil, 'no function is returned on the first call')
+
+-- Now run this trace when `closure()` is defined and has an open
+-- local upvalue.
+func_with_uv = wrapped_trace(true)
+assert(type(func_with_uv) == 'function',
+       'function is returned after the second call')
+
+test:is(func_with_uv(), EXPECTED, 'correct result of the closure call')
+
+test:done(true)
-- 
2.43.0


             reply	other threads:[~2024-02-07 14:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-07 14:51 Sergey Kaplun via Tarantool-patches [this message]
2024-02-08 11:11 ` Sergey Bronnikov via Tarantool-patches
2024-02-08 11:24   ` Sergey Kaplun via Tarantool-patches
2024-02-08 12:49     ` Sergey Bronnikov via Tarantool-patches
2024-02-09 16:54 ` Maxim Kokryashkin via Tarantool-patches
2024-02-15 13:48 ` 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=20240207145108.29786-1-skaplun@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=m.kokryashkin@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH luajit] Consider slots used by upvalues in use-def analysis.' \
    /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