Re: [Tarantool-patches] [PATCH luajit v3 2/2] Fix snapshot PC when linking to BC_JLOOP that was a BC_RET*.

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

Hi, Sergey!
Thanks for the review!
Fixed your comments, the branch is force-pushed.

You can find the diff below.
On Tue, Oct 10, 2023 at 11:14:36AM +0300, Sergey Kaplun wrote:
> Hi, Maksim!
> Thanks for the patch!
> LGTM, just a few nits regarding to the comments in the test.
> 
> On 04.10.23, Maksim Kokryashkin wrote:
> > From: Mike Pall <mike>
> > 
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
> 
> <snipped>
> 
> > +--
> > +-- ---- TRACE 2 exit 1
> > +-- ---- TRACE 4 start 2/1 test.lua:3
> > +-- 0013  RET1     1   2
> > +-- 0012  ADDVV    1   1   2
> > +-- 0013  JLOOP    3   3
> > +--
> > +-- During the recording of the latter JLOOP the assertion added
> > +-- in the patch is triggered.
> 
> Minor: I suggest to rephrase this as the following:
> 
> | The assertion introduced in the previous patch is triggered during
> | recording of the last 0013 JLOOP.
> 
> > +--
> > +-- See also:
> > +-- https://github.com/luaJIT/LuaJIT/issues/624
> > +
> > +
> 
> Nit: extra empty line.
Diff with changes:
===
diff --git a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
index 726b2efa..565e4cbf 100644
--- a/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
+++ b/test/tarantool-tests/lj-624-jloop-snapshot-pc.test.lua
@@ -66,13 +66,12 @@ test:plan(1)
 -- 0012  ADDVV    1   1   2
 -- 0013  JLOOP    3   3
 --
--- During the recording of the latter JLOOP the assertion added
--- in the patch is triggered.
+-- The assertion introduced in the previous patch is triggered during
+-- recording of the last 0013 JLOOP.
 --
 -- See also:
 -- https://github.com/luaJIT/LuaJIT/issues/624
 
-
 jit.opt.start('hotloop=1', 'hotexit=1')
 local function fib(n)
   return n < 2 and n or fib(n - 1) + fib(n - 2)
===

> 
> > +jit.opt.start('hotloop=1', 'hotexit=1')
> > +local function fib(n)
> > +  return n < 2 and n or fib(n - 1) + fib(n - 2)
> > +end
> > +
> > +fib(5)
> > +
> > +test:ok(true, 'snapshot pc is correct')
> > +test:done(true)
> > -- 
> > 2.39.3 (Apple Git-145)
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun