[Tarantool-patches] [PATCH luajit] LJ_GC64: Always snapshot functions for non-base frames.
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Tue Oct 17 17:56:45 MSK 2023
Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Thu, Oct 12, 2023 at 04:06:25PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: Sergey Bronnikov <sergeyb at tarantool.org>
>
> Reported by Arseny Vakhrushev.
> Analysis and fix contributed by Peter Cawley.
>
> (cherry picked from commit ff1e72acead01df7d8ed0fbb31efd32f57953618)
>
> The problem is GC64-specific and could be reproduced with enabled
> compiler options LUA_USE_ASSERT and LUA_USE_APICHECK.
I would prefer to see a bug description in the commit message too.
>
> Sergey Kaplun:
> * minimized reproducer made by fuzzing test
>
> Sergey Bronnikov:
> * added the description (see a comment in the test)
> * added tests for the problem: first one based on the original
> reproducer and second one based on a reproducer made by fuzzing test.
>
> Part of tarantool/tarantool#8825
Should be tarantool/tarantool#9145
> ---
> Branch: https://github.com/tarantool/luajit/commits/ligurio/lj-611-always-snapshot-functions-for-non-base-frames
> PR: https://github.com/tarantool/tarantool/pull/9254
> LJ issue: https://github.com/LuaJIT/LuaJIT/issues/611
>
> src/lj_record.c | 1 +
> src/lj_snap.c | 9 +++-
> ...t-functions-for-non-base-frames-1.test.lua | 36 +++++++++++++++
> ...hot-functions-for-non-base-frames.test.lua | 45 +++++++++++++++++++
> 4 files changed, 89 insertions(+), 2 deletions(-)
> create mode 100644 test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua
> create mode 100644 test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua
>
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 48a5481b..55785e23 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -211,6 +211,7 @@ static TRef getcurrf(jit_State *J)
> {
> if (J->base[-1-LJ_FR2])
> return J->base[-1-LJ_FR2];
> + /* Non-base frame functions ought to be loaded already. */
> lj_assertJ(J->baseslot == 1+LJ_FR2, "bad baseslot");
> return sloadt(J, -1-LJ_FR2, IRT_FUNC, IRSLOAD_READONLY);
> }
> diff --git a/src/lj_snap.c b/src/lj_snap.c
> index 6c5e5e53..06ae17eb 100644
> --- a/src/lj_snap.c
> +++ b/src/lj_snap.c
> @@ -85,8 +85,13 @@ static MSize snapshot_slots(jit_State *J, SnapEntry *map, BCReg nslots)
> IRIns *ir = &J->cur.ir[ref];
> if ((LJ_FR2 || !(sn & (SNAP_CONT|SNAP_FRAME))) &&
> ir->o == IR_SLOAD && ir->op1 == s && ref > retf) {
> - /* No need to snapshot unmodified non-inherited slots. */
> - if (!(ir->op2 & IRSLOAD_INHERIT))
> + /*
> + ** No need to snapshot unmodified non-inherited slots.
> + ** But always snapshot the function below a frame in LJ_FR2 mode.
> + */
> + if (!(ir->op2 & IRSLOAD_INHERIT) &&
> + (!LJ_FR2 || s == 0 || s+1 == nslots ||
> + !(J->slot[s+1] & (TREF_CONT|TREF_FRAME))))
> continue;
> /* No need to restore readonly slots and unmodified non-parent slots. */
> if (!(LJ_DUALNUM && (ir->op2 & IRSLOAD_CONVERT)) &&
> diff --git a/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua
> new file mode 100644
> index 00000000..759c2862
> --- /dev/null
> +++ b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames-1.test.lua
I believe we should give these tests better names that <name>-1.test.lua
and <name>.test.lua. Maybe those names should include the original source of
the reproducer (I mean, fuzzer, and the original issue).
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames-1'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +-- GC64: Function missing in snapshot for non-base frame
> +-- https://github.com/LuaJIT/LuaJIT/issues/611
> +
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +local inner_counter = 0
> +local SIDE_START = 1
> +-- Lower frame to return from `inner()` function side trace.
> +-- TODO: Give a reason for vararg func.
> +local function lower_frame(...)
> + local inner = function()
> + if inner_counter > SIDE_START then
> + return
> + end
> + inner_counter = inner_counter + 1
> + end
> + inner(..., inner(inner()))
> +end
> +
> +-- Compile `inner()` function.
> +lower_frame()
> +lower_frame()
> +-- Compile hotexit
> +lower_frame()
> +-- Take side exit from side trace.
> +lower_frame(1)
> +
> +test:ok(true, 'function is present in snapshot')
> +test:done(true)
> diff --git a/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua
> new file mode 100644
> index 00000000..7305c185
> --- /dev/null
> +++ b/test/tarantool-tests/lj-611-always-snapshot-functions-for-non-base-frames.test.lua
> @@ -0,0 +1,45 @@
> +local tap = require('tap')
> +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +-- Test reproduces a bug "GC64: Function missing in snapshot for non-base
Typo: s/Test/The test/
Typo: s/a bug/the bug/
> +-- frame" [1], and based on reproducer described in [2].
Typo: s/based/is based/
Typo: s/reproducer/the reproducer/
> +--
> +-- [1]: https://github.com/LuaJIT/LuaJIT/issues/611
> +-- [2]: https://github.com/LuaJIT/LuaJIT/issues/611#issuecomment-679228156
> +--
> +-- Function `outer` is recorded to a trace and calls a builtin function that is
Typo: s/builtin/built-in/
> +-- not JIT-compilable and therefore triggers exit to interpreter, and then it
Typo: s/to interpreter/to the interpreter
> +-- resumes tracing just after the call returns - this is a trace stitching.
Typo: s/is a/is/
> +-- Then, within the call, we need the potential for a side trace. Finally, we need
> +-- that side exit to be taken enough for the exit to be compiled into a trace.
Typo: s/taken enough/taken enough times/
> +-- The loop at the bottom has enough iterations to trigger JIT compilation, and
> +-- enough more on top on trigger compilation of the not case. Compilation of
These two lines are incomprehensible. Please parahphrase them.
> +-- this case hits the assertion failure.
> +
> +local inner
> +for _ = 1, 3 do
> + inner = function(_, i)
> + return i < 4
> + end
> +end
> +
> +local function outer(i)
> + -- The function `string.gsub` is not JIT-compilable and triggers a trace
> + -- exit. For example, `string.gmatch` and `string.match` are suitable as
> + -- well.
> + -- See https://github.com/tarantool/tarantool/wiki/LuaJIT-Not-Yet-Implemented.
> + inner(string.gsub('', '', ''), i)
> +end
> +
> +for i = 1, 4 do
> + outer(i)
> +end
I like this repro much more. It is easier to understand than the first one.
What's the motivation behind having two tests for the same issue?
> +
> +test:ok(true, 'function is present in snapshot')
> +test:done(true)
> --
> 2.34.1
>
More information about the Tarantool-patches
mailing list