Re: [Tarantool-patches] [PATCH luajit 2/2] Fix maxslots when recording BC_VARG, part 2.

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

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

On 14.07.23, Maxim Kokryashkin wrote:
> 
> Hi!
> Thanks for the patch!
> LGTM, except for a few nits below.
> > 
> >>From: Mike Pall <mike>
> >>
> >>Analyzed by Sergey Kaplun.
> >>
> >>(cherry-picked from commit a01cba9d2d74efc57376822aa43db2d5043af5a4)
> >>
> >>This patch is the follow-up for the previous one. It removes the
> >Typo: s/the/a
> >Typo: s/for the/to the/

Fixed, thanks!

> >>condition for `maxslot` changing in the case when varargs are defined
> >>on the trace (i.e. `framedepth` > 0).
> >Typo: s/i.e./i.e.,/

I prefer to avoid double punctuation symbols as mentioned here[1].
Also, this style is allowed by Oxford dictionary (as referenced in
[1]).

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8825
> >>---
> >> src/lj_record.c | 8 ++------
> >> .../lj-1024-varg-maxslot.test.lua | 19 ++++++++++++++++++-
> >> 2 files changed, 20 insertions(+), 7 deletions(-)
> >>
> >>diff --git a/src/lj_record.c b/src/lj_record.c
> >>index 112524d3..02d9db9e 100644
> >>--- a/src/lj_record.c
> >>+++ b/src/lj_record.c
> >>@@ -1775,12 +1775,8 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
> >>   if (J->framedepth > 0) { /* Simple case: varargs defined on-trace. */
> >>     ptrdiff_t i;
> >>     if (nvararg < 0) nvararg = 0;
> >>- if (nresults == -1) {
> >>- nresults = nvararg;
> >>- J->maxslot = dst + (BCReg)nvararg;
> >>- } else if (dst + nresults > J->maxslot) {
> >>- J->maxslot = dst + (BCReg)nresults;
> >>- }
> >>+ if (nresults == -1) nresults = nvararg;
> >>+ J->maxslot = dst + (BCReg)nresults;
> >>     for (i = 0; i < nresults; i++)
> >>       J->base[dst+i] = i < nvararg ? getslot(J, i - nvararg - 1 - LJ_FR2) : TREF_NIL;
> >>   } else { /* Unknown number of varargs passed to trace. */
> >>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> >>index 14270595..f8d74e8a 100644
> >>--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> >>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> >>@@ -3,7 +3,7 @@ local test = tap.test('lj-noticket-varg-usedef'):skipcond({
> >>   ['Test requires JIT enabled'] = not jit.status(),
> >> })
> >> 
> >>-test:plan(1)
> >>+test:plan(2)
> >> 
> >> jit.opt.start('hotloop=1')
> >> 
> >>@@ -20,4 +20,21 @@ end
> >> 
> >> test:ok(true, 'BC_VARG recording 0th frame depth')
> >> 
> >>+-- Now the same case, but with additional frame, so VARG slots
> >Typo: s/with/with an/

Fixed, thanks!

> >>+-- are defined on the trace.
> >>+local function bump_varg_frame(...)
> >Nit: Not sure about the name here, maybe it is better to call it
> >just `varg_frame` or something like that. Feel free to ignore, though.

Changed to the `varg_frame()`.

See the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
index f8d74e8a..cdefbe83 100644
--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
@@ -20,9 +20,9 @@ end
 
 test:ok(true, 'BC_VARG recording 0th frame depth')
 
--- Now the same case, but with additional frame, so VARG slots
+-- Now the same case, but with an additional frame, so VARG slots
 -- are defined on the trace.
-local function bump_varg_frame(...)
+local function varg_frame(...)
   -- BC_VARG 1 1 0. `...` is nil (argument for the script).
   -- luacheck: ignore
   -- XXX: some condition to use several slots on the Lua stack.
@@ -32,7 +32,7 @@ end
 counter = 0
 while counter < 3 do
   counter = counter + 1
-  bump_varg_frame()
+  varg_frame()
 end
 
 test:ok(true, 'BC_VARG recording with defined on trace VARG slots')
===================================================================

> >>+ -- BC_VARG 1 1 0. `...` is nil (argument for the script).
> >>+ -- luacheck: ignore
> >>+ -- XXX: some condition to use several slots on the Lua stack.
> >>+ anchor = 1 >= 1, ...
> >>+end
> >>+
> >>+counter = 0
> >>+while counter < 3 do
> >>+ counter = counter + 1
> >>+ bump_varg_frame()
> >>+end
> >>+
> >>+test:ok(true, 'BC_VARG recording with defined on trace VARG slots')
> >>+
> >> os.exit(test:check() and 0 or 1)
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

[1]: https://english.stackexchange.com/questions/352253/why-does-oxforddictionaries-com-not-use-a-comma-after-the-abbreviations-i-e-a

-- 
Best regards,
Sergey Kaplun