Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Always snapshot functions for non-base frames.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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
>