[Tarantool-patches] [PATCH luajit 0/2] Fix maxslots when recording BC_VARG

[Tarantool-patches] [PATCH luajit 0/2] Fix maxslots when recording BC_VARG

[Sergey Kaplun via Tarantool-patches]

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1024-varg-maxslot
Tarantool PR: https://github.com/tarantool/tarantool/pull/8865
Related issues:
* https://github.com/tarantool/tarantool/issues/8825
* https://github.com/LuaJIT/LuaJIT/issues/1024

Mike Pall (2):
  Fix maxslots when recording BC_VARG.
  Fix maxslots when recording BC_VARG, part 2.

 src/lj_record.c                               | 11 ++---
 .../lj-1024-varg-maxslot.test.lua             | 40 +++++++++++++++++++
 2 files changed, 43 insertions(+), 8 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1024-varg-maxslot.test.lua

-- 
2.34.1

[Tarantool-patches] [PATCH luajit 1/2] Fix maxslots when recording BC_VARG.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Analyzed by Sergey Kaplun.

(cherry-picked from commit 94ada59628dd6ce5d6d2dad1d35a68ad30127f53)

While recording BC_VARG `J->maxslot` isn't shrunk to the effective stack
top. This leads to dead value stored in the JIT slots and the following
assertion failure for these slots check in `rec_check_slots()`. Note,
that `rec_varg()` modifies `maxslot` only under the condition that
`maxslot` should be increased, but the dead values are left for the
opposite case.

This patch removes the condition inside `rec_varg()` only for the case
when varargs are not defined on trace (`framedepth` is 0), but the
similar issue still occurs for the case when vararg are defined on the
trace.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8825
---
 src/lj_record.c                               |  3 +--
 .../lj-1024-varg-maxslot.test.lua             | 23 +++++++++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1024-varg-maxslot.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index a90cba77..112524d3 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -1812,8 +1812,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
       }
       for (i = nvararg; i < nresults; i++)
 	J->base[dst+i] = TREF_NIL;
-      if (dst + (BCReg)nresults > J->maxslot)
-	J->maxslot = dst + (BCReg)nresults;
+      J->maxslot = dst + (BCReg)nresults;
     } else if (select_detect(J)) {  /* y = select(x, ...) */
       TRef tridx = J->base[dst-1];
       TRef tr = TREF_NIL;
diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
new file mode 100644
index 00000000..14270595
--- /dev/null
+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
@@ -0,0 +1,23 @@
+local tap = require('tap')
+local test = tap.test('lj-noticket-varg-usedef'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+jit.opt.start('hotloop=1')
+
+local counter = 0
+-- luacheck: ignore
+local anchor
+while counter < 3 do
+  counter = counter + 1
+  -- BC_VARG 5 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
+
+test:ok(true, 'BC_VARG recording 0th frame depth')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

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

[Sergey Kaplun via Tarantool-patches]

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
condition for `maxslot` changing in the case when varargs are defined
on the trace (i.e. `framedepth` > 0).

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
+-- are defined on the trace.
+local function bump_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.
+  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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2759 bytes --]


Hi!
Thanks for the patch!
LGTM, except for a few nits regarding the commit message.
 
> 
>>From: Mike Pall <mike>
>>
>>Analyzed by Sergey Kaplun.
>>
>>(cherry-picked from commit 94ada59628dd6ce5d6d2dad1d35a68ad30127f53)
>>
>>While recording BC_VARG `J->maxslot` isn't shrunk to the effective stack
>Typo: s/shrunk/shrinking
>>top. This leads to dead value stored in the JIT slots and the following
>Typo: s/value/values/
>>assertion failure for these slots check in `rec_check_slots()`. Note,
>>that `rec_varg()` modifies `maxslot` only under the condition that
>>`maxslot` should be increased, but the dead values are left for the
>>opposite case.
>>
>>This patch removes the condition inside `rec_varg()` only for the case
>>when varargs are not defined on trace (`framedepth` is 0), but the
>>similar issue still occurs for the case when vararg are defined on the
>Typo: s/vararg/varagrs/
>>trace.
>>
>>Sergey Kaplun:
>>* added the description and the test for the problem
>>
>>Part of tarantool/tarantool#8825
>>---
>> src/lj_record.c | 3 +--
>> .../lj-1024-varg-maxslot.test.lua | 23 +++++++++++++++++++
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>> create mode 100644 test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>
>>diff --git a/src/lj_record.c b/src/lj_record.c
>>index a90cba77..112524d3 100644
>>--- a/src/lj_record.c
>>+++ b/src/lj_record.c
>>@@ -1812,8 +1812,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>>       }
>>       for (i = nvararg; i < nresults; i++)
>>  J->base[dst+i] = TREF_NIL;
>>- if (dst + (BCReg)nresults > J->maxslot)
>>- J->maxslot = dst + (BCReg)nresults;
>>+ J->maxslot = dst + (BCReg)nresults;
>>     } else if (select_detect(J)) { /* y = select(x, ...) */
>>       TRef tridx = J->base[dst-1];
>>       TRef tr = TREF_NIL;
>>diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>new file mode 100644
>>index 00000000..14270595
>>--- /dev/null
>>+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
>>@@ -0,0 +1,23 @@
>>+local tap = require('tap')
>>+local test = tap.test('lj-noticket-varg-usedef'):skipcond({
>>+ ['Test requires JIT enabled'] = not jit.status(),
>>+})
>>+
>>+test:plan(1)
>>+
>>+jit.opt.start('hotloop=1')
>>+
>>+local counter = 0
>>+-- luacheck: ignore
>>+local anchor
>>+while counter < 3 do
>>+ counter = counter + 1
>>+ -- BC_VARG 5 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
>>+
>>+test:ok(true, 'BC_VARG recording 0th frame depth')
>>+
>>+os.exit(test:check() and 0 or 1)
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin

[-- Attachment #2: Type: text/html, Size: 3983 bytes --]

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]


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/
>>condition for `maxslot` changing in the case when varargs are defined
>>on the trace (i.e. `framedepth` > 0).
>Typo: s/i.e./i.e.,/
>>
>>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/
>>+-- 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.
>>+ -- 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
> 

[-- Attachment #2: Type: text/html, Size: 4516 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Kaplun via Tarantool-patches]

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 regarding the commit message.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Analyzed by Sergey Kaplun.
> >>
> >>(cherry-picked from commit 94ada59628dd6ce5d6d2dad1d35a68ad30127f53)
> >>
> >>While recording BC_VARG `J->maxslot` isn't shrunk to the effective stack
> >Typo: s/shrunk/shrinking

I meant the third form shrink/shrank/shrunk here. Ignoring.

> >>top. This leads to dead value stored in the JIT slots and the following
> >Typo: s/value/values/

Fixed.

> >>assertion failure for these slots check in `rec_check_slots()`. Note,
> >>that `rec_varg()` modifies `maxslot` only under the condition that
> >>`maxslot` should be increased, but the dead values are left for the
> >>opposite case.
> >>
> >>This patch removes the condition inside `rec_varg()` only for the case
> >>when varargs are not defined on trace (`framedepth` is 0), but the
> >>similar issue still occurs for the case when vararg are defined on the
> >Typo: s/vararg/varagrs/
> >>trace.

Fixed.

> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8825

<snipped>

> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]


Hi!
Thanks for the fixes!
LGTM
 
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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 regarding the commit message.
>>>  
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Analyzed by Sergey Kaplun.
>>> >>
>>> >>(cherry-picked from commit 94ada59628dd6ce5d6d2dad1d35a68ad30127f53)
>>> >>
>>> >>While recording BC_VARG `J->maxslot` isn't shrunk to the effective stack
>>> >Typo: s/shrunk/shrinking
>>
>>I meant the third form shrink/shrank/shrunk here. Ignoring.
>>
>>> >>top. This leads to dead value stored in the JIT slots and the following
>>> >Typo: s/value/values/
>>
>>Fixed.
>>
>>> >>assertion failure for these slots check in `rec_check_slots()`. Note,
>>> >>that `rec_varg()` modifies `maxslot` only under the condition that
>>> >>`maxslot` should be increased, but the dead values are left for the
>>> >>opposite case.
>>> >>
>>> >>This patch removes the condition inside `rec_varg()` only for the case
>>> >>when varargs are not defined on trace (`framedepth` is 0), but the
>>> >>similar issue still occurs for the case when vararg are defined on the
>>> >Typo: s/vararg/varagrs/
>>> >>trace.
>>
>>Fixed.
>>
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8825
>>
>><snipped>
>>
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 2437 bytes --]

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5032 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>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
> 

[-- Attachment #2: Type: text/html, Size: 6944 bytes --]

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! The commit message is OK after fixing Max nits,
but I still have some questions regarding the test.

On 10.07.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>

<snipped>

> diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> new file mode 100644
> index 00000000..14270595
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> @@ -0,0 +1,23 @@
> +local tap = require('tap')
> +local test = tap.test('lj-noticket-varg-usedef'):skipcond({

Now you have a ticket number.

> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +jit.opt.start('hotloop=1')
> +
> +local counter = 0
> +-- luacheck: ignore
> +local anchor
> +while counter < 3 do
> +  counter = counter + 1
> +  -- BC_VARG 5 1 0. `...` is nil (argument for the script).
> +  -- luacheck: ignore
> +  -- XXX: some condition to use several slots on the Lua stack.
> +  anchor = 1 >= 1, ...

Well, I have no idea, why this black voodoo magic is required. Comment
doesn't make it clear either. It would be nice to describe the purpose
of this in a more verbose way.

> +end
> +
> +test:ok(true, 'BC_VARG recording 0th frame depth')
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! Again, everything is fine with the commit message
after resolving the comments left by Max, but the black voodoo magic
condition is still unclear (see my comments to the first patch).

On 10.07.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 

<snipped>

> 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

<snipped>

> @@ -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
> +-- are defined on the trace.
> +local function bump_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.
> +  anchor = 1 >= 1, ...

Clarification is required here as well as for the first test.

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

Minor: I believe it should be "BC_VARG recording with VARG slots defined
on the trace", but I might be missing something.

> +
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.34.1
> 

-- 
Best regards,
IM

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

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the review!

On 18.07.23, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! The commit message is OK after fixing Max nits,
> but I still have some questions regarding the test.
> 
> On 10.07.23, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> 
> <snipped>
> 
> > diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> > new file mode 100644
> > index 00000000..14270595
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> > @@ -0,0 +1,23 @@
> > +local tap = require('tap')
> > +local test = tap.test('lj-noticket-varg-usedef'):skipcond({
> 
> Now you have a ticket number.

Fixed, thanks!

> 
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(1)
> > +
> > +jit.opt.start('hotloop=1')
> > +
> > +local counter = 0
> > +-- luacheck: ignore
> > +local anchor
> > +while counter < 3 do
> > +  counter = counter + 1
> > +  -- BC_VARG 5 1 0. `...` is nil (argument for the script).
> > +  -- luacheck: ignore
> > +  -- XXX: some condition to use several slots on the Lua stack.
> > +  anchor = 1 >= 1, ...
> 
> Well, I have no idea, why this black voodoo magic is required. Comment
> doesn't make it clear either. It would be nice to describe the purpose
> of this in a more verbose way.

Added more verbose description. See the full diff below. Branch is force
pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
index 14270595..9a968b0c 100644
--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
@@ -1,5 +1,5 @@
 local tap = require('tap')
-local test = tap.test('lj-noticket-varg-usedef'):skipcond({
+local test = tap.test('lj-1024-varg-usedef'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
@@ -13,8 +13,20 @@ local anchor
 while counter < 3 do
   counter = counter + 1
   -- BC_VARG 5 1 0. `...` is nil (argument for the script).
+  -- We have the following bytecodes to be recorded:
+  -- 0031  ADDVN    2   2   0  ; 1
+  -- 0032  KSHORT   4   1
+  -- 0033  KSHORT   5   1
+  -- 0034  ISLE     4   5
+  -- 0035  JMP      4 => 0038
+  -- 0038  KPRI     4   2
+  -- 0039  VARG     5   1   0
+  --
+  -- 0033 KSHORT bytecode uses the 6th JIT slot and the 5th Lua
+  -- slot. This Lua slot will be set to nil after 0039 VARG
+  -- bytecode execution, so after VARG recording maxslot should
+  -- point to the 5th JIT slot.
   -- luacheck: ignore
-  -- XXX: some condition to use several slots on the Lua stack.
   anchor = 1 >= 1, ...
 end
 
===================================================================

> 
> > +end
> > +
> > +test:ok(true, 'BC_VARG recording 0th frame depth')
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the review!

On 18.07.23, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! Again, everything is fine with the commit message
> after resolving the comments left by Max, but the black voodoo magic
> condition is still unclear (see my comments to the first patch).
> 
> On 10.07.23, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> 
> <snipped>
> 
> > 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
> 
> <snipped>
> 
> > @@ -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
> > +-- are defined on the trace.
> > +local function bump_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.
> > +  anchor = 1 >= 1, ...
> 
> Clarification is required here as well as for the first test.

Fixed!

> 
> > +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')
> 
> Minor: I believe it should be "BC_VARG recording with VARG slots defined
> on the trace", but I might be missing something.

Fixed, as you suggested. See the iterative patch below, branch is
force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
index ec8afbb5..e33ddba2 100644
--- a/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
+++ b/test/tarantool-tests/lj-1024-varg-maxslot.test.lua
@@ -36,8 +36,19 @@ test:ok(true, 'BC_VARG recording 0th frame depth')
 -- are defined on the trace.
 local function varg_frame(...)
   -- BC_VARG 1 1 0. `...` is nil (argument for the script).
+  -- We have the following bytecodes to be recorded:
+  -- 0001  . . KSHORT   0   1
+  -- 0002  . . KSHORT   1   1
+  -- 0003  . . ISLE     0   1
+  -- 0004  . . JMP      0 => 0007
+  -- 0007  . . KPRI     0   2
+  -- 0008  . . VARG     1   1   0
+  --
+  -- 0002 KSHORT bytecode uses the 2nd JIT slot and the 1st Lua
+  -- slot. This Lua slot will be set to nil after 0008 VARG
+  -- bytecode execution, so after VARG recording maxslot should
+  -- point to the 1st JIT slot.
   -- luacheck: ignore
-  -- XXX: some condition to use several slots on the Lua stack.
   anchor = 1 >= 1, ...
 end
 
@@ -47,6 +58,6 @@ while counter < 3 do
   varg_frame()
 end
 
-test:ok(true, 'BC_VARG recording with defined on trace VARG slots')
+test:ok(true, 'BC_VARG recording with VARG slots defined on trace')
 
 os.exit(test:check() and 0 or 1)
===================================================================

> 
> > +
> >  os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the fixes! LGTM now.

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the fixes! LGTM now.

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, release/2.11 and
release/2.10.

On 10.07.23, Sergey Kaplun wrote:
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1024-varg-maxslot
> Tarantool PR: https://github.com/tarantool/tarantool/pull/8865
> Related issues:
> * https://github.com/tarantool/tarantool/issues/8825
> * https://github.com/LuaJIT/LuaJIT/issues/1024
> 
> Mike Pall (2):
>   Fix maxslots when recording BC_VARG.
>   Fix maxslots when recording BC_VARG, part 2.
> 
>  src/lj_record.c                               | 11 ++---
>  .../lj-1024-varg-maxslot.test.lua             | 40 +++++++++++++++++++
>  2 files changed, 43 insertions(+), 8 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1024-varg-maxslot.test.lua
> 
> -- 
> 2.34.1
> 

-- 
Best regards,
IM