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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.

On 12.10.23, Sergey Bronnikov 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.
> 
> 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
> ---
> 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

<snipped>

> 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
> @@ -0,0 +1,36 @@
> +local tap = require('tap')
> +local test = tap.test('lj-611-always-snapshot-functions-for-non-base-frames-1'):skipcond({

I suppose, that we can choose smaller name, because the code width is
more than 80 symbols. :)

I suggest 'lj-611-gc64-inherit-frame-slot' for both tests and
filenames. Feel free to change it as you wish:).

> +  ['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.

I see this TODO, but don't see an explanation.
If you want to observe the behaviour compare the code flow (with the
fixed-arg function) in the gdb.

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

Missed dot at the end of the sentence.

> +lower_frame()
> +-- Take side exit from side trace.
> +lower_frame(1)
> +
> +test:ok(true, 'function is present in snapshot')

Typo: s/snapshot/the 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({

I suppose, that we can choose smaller name, because the code width is
more than 80 symbols. :)

I suggest 'lj-611-gc64-inherit-frame-slot-orig' for both tests and
filenames. Feel free to change it as you wish:).

> +  ['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
> +-- frame" [1], and based on reproducer described in [2].
> +--
> +-- [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
> +-- not JIT-compilable and therefore triggers exit to interpreter, and then it
> +-- resumes tracing just after the call returns - this is a trace stitching.
> +-- 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.
> +-- 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
> +-- 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.

Comment width is more than 66 symbols.

> +  inner(string.gsub('', '', ''), i)
> +end
> +
> +for i = 1, 4 do
> +  outer(i)
> +end
> +
> +test:ok(true, 'function is present in snapshot')

Typo: s/snapshot/the snapshot/

> +test:done(true)
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun