[Tarantool-patches] [PATCH luajit] LJ_GC64: Always snapshot functions for non-base frames.
Sergey Kaplun
skaplun at tarantool.org
Mon Oct 23 17:57:01 MSK 2023
Hi, Sergey!
Thanks for the patch!
Please, consider my comments below.
On 12.10.23, Sergey Bronnikov 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.
>
> 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
More information about the Tarantool-patches
mailing list