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

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

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Thanks to Peter Cawley.

(cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)

In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
`maxslot` value was unconditionally set to the destination slot, so some
top slots may be omitted in the snapshot entry. Since these slots are
omitted, they are not restored correctly, when restoring from snapshot
for this side exit.

This patch adds the check for the aforementioned case, to avoid maxslot
shrinking.

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

Part of tarantool/tarantool#8825
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
PR: https://github.com/tarantool/tarantool/pull/8986
Related issues:
* https://github.com/LuaJIT/LuaJIT/issues/1046
* https://github.com/tarantool/tarantool/issues/8825

 src/lj_record.c                               | 12 +++-
 .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
 2 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua

diff --git a/src/lj_record.c b/src/lj_record.c
index 34d1210a..6bcdb04c 100644
--- a/src/lj_record.c
+++ b/src/lj_record.c
@@ -1807,8 +1807,12 @@ 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)nresults;
+    if (nresults != 1) {
+      if (nresults == -1) nresults = nvararg;
+      J->maxslot = dst + (BCReg)nresults;
+    } else if (dst >= J->maxslot) {
+      J->maxslot = dst + 1;
+    }
     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. */
@@ -1840,7 +1844,9 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
       }
       for (i = nvararg; i < nresults; i++)
 	J->base[dst+i] = TREF_NIL;
-      J->maxslot = dst + (BCReg)nresults;
+      if (nresults != 1 || dst >= J->maxslot) {
+	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-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
new file mode 100644
index 00000000..34c5c572
--- /dev/null
+++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
@@ -0,0 +1,58 @@
+local tap = require('tap')
+local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(2)
+
+jit.opt.start('hotloop=1')
+
+-- luacheck: ignore
+local anchor
+local N_ITER = 5
+local SIDE_ITER = N_ITER - 1
+for i = 1, N_ITER do
+  -- In case, when `BC_VARG` set the VARG slot to the non-top
+  -- stack slot, `maxslot` value was unconditionally set to the
+  -- destination slot, so the following snapshot is used:
+  -- SNAP   #4   [ ---- ---- ---- nil  ]
+  -- instead of:
+  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
+  -- Since these slots are omitted, they are not restored
+  -- correctly, when restoring from snapshot for this side exit.
+  anchor = ...
+  if i > SIDE_ITER then
+    -- XXX: Don't use `test:ok()` here to avoid double-running of
+    -- tests in case of `i` incorrect restoring from the snapshot.
+    assert(i > SIDE_ITER)
+  end
+end
+
+test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
+
+-- Now the same case, but with an additional frame, so VARG slots
+-- are defined on the trace.
+local function varg_frame(anchor, i, side_iter, ...)
+  anchor = ...
+  -- In case, when `BC_VARG` set the VARG slot to the non-top
+  -- stack slot, `maxslot` value was unconditionally set to the
+  -- destination slot, so the following snapshot is used:
+  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
+  -- instead of:
+  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
+  -- Since these slots are omitted, they are not restored
+  -- correctly, when restoring from snapshot for this side exit.
+  if i > side_iter then
+    -- XXX: Don't use `test:ok()` here to avoid double-running of
+    -- tests in case of `i` incorrect restoring from the snapshot.
+    assert(i > side_iter)
+  end
+end
+
+for i = 1, N_ITER do
+  varg_frame(nil, i, SIDE_ITER)
+end
+
+test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result')
+
+test:done(true)
-- 
2.41.0

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

[Maxim Kokryashkin via Tarantool-patches]

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

On Tue, Aug 15, 2023 at 03:32:15PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
> 
> In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
Typo: s/set/sets/
> `maxslot` value was unconditionally set to the destination slot, so some
> top slots may be omitted in the snapshot entry. Since these slots are
> omitted, they are not restored correctly, when restoring from snapshot
Typo: s/snapshot/a snapshot/
> for this side exit.
> 
> This patch adds the check for the aforementioned case, to avoid maxslot
> shrinking.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> PR: https://github.com/tarantool/tarantool/pull/8986
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1046
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_record.c                               | 12 +++-
>  .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> 
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 34d1210a..6bcdb04c 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -1807,8 +1807,12 @@ 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)nresults;
> +    if (nresults != 1) {
> +      if (nresults == -1) nresults = nvararg;
> +      J->maxslot = dst + (BCReg)nresults;
> +    } else if (dst >= J->maxslot) {
> +      J->maxslot = dst + 1;
> +    }
>      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. */
> @@ -1840,7 +1844,9 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>        }
>        for (i = nvararg; i < nresults; i++)
>  	J->base[dst+i] = TREF_NIL;
> -      J->maxslot = dst + (BCReg)nresults;
> +      if (nresults != 1 || dst >= J->maxslot) {
> +	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-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> new file mode 100644
> index 00000000..34c5c572
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> @@ -0,0 +1,58 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1')
> +
> +-- luacheck: ignore
> +local anchor
> +local N_ITER = 5
> +local SIDE_ITER = N_ITER - 1
> +for i = 1, N_ITER do
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> +  -- instead of:
> +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
Snapshot examples here ceratinly give the idea of what goes wrong,
but the `0009` and `0001` are meaningless by themselves. I think it would be
nice to include IRs here too.
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
Please fix the same typos as in the commit message here.
> +  anchor = ...
> +  if i > SIDE_ITER then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
I think better phrasing would be:
| `test:ok()` is not used here ...
> +    -- tests in case of `i` incorrect restoring from the snapshot.
Typo: s/restoring/restoration/
> +    assert(i > SIDE_ITER)
> +  end
> +end
> +
> +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
> +
> +-- Now the same case, but with an additional frame, so VARG slots
> +-- are defined on the trace.
> +local function varg_frame(anchor, i, side_iter, ...)
> +  anchor = ...
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> +  -- instead of:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
I guess we don't need to repeat the entire comment again.
> +  if i > side_iter then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> +    -- tests in case of `i` incorrect restoring from the snapshot.
Same typos as in the comment above.
> +    assert(i > side_iter)
> +  end
> +end
> +
> +for i = 1, N_ITER do
> +  varg_frame(nil, i, SIDE_ITER)
> +end
> +
> +test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result')
> +
> +test:done(true)
> -- 
> 2.41.0
> 

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

[Sergey Kaplun via Tarantool-patches]

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

On 16.08.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Tue, Aug 15, 2023 at 03:32:15PM +0300, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Thanks to Peter Cawley.
> > 
> > (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
> > 
> > In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
> Typo: s/set/sets/

Fixed.

> > `maxslot` value was unconditionally set to the destination slot, so some
> > top slots may be omitted in the snapshot entry. Since these slots are
> > omitted, they are not restored correctly, when restoring from snapshot
> Typo: s/snapshot/a snapshot/

Fixed.

> > for this side exit.
> > 
> > This patch adds the check for the aforementioned case, to avoid maxslot
> > shrinking.
> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Part of tarantool/tarantool#8825
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> > PR: https://github.com/tarantool/tarantool/pull/8986
> > Related issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/1046
> > * https://github.com/tarantool/tarantool/issues/8825
> > 
> >  src/lj_record.c                               | 12 +++-
> >  .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
> >  2 files changed, 67 insertions(+), 3 deletions(-)
> >  create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > 
> > diff --git a/src/lj_record.c b/src/lj_record.c
> > index 34d1210a..6bcdb04c 100644
> > --- a/src/lj_record.c
> > +++ b/src/lj_record.c

<snipped>

> > diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > new file mode 100644
> > index 00000000..34c5c572
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > @@ -0,0 +1,58 @@
> > +local tap = require('tap')
> > +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +test:plan(2)
> > +
> > +jit.opt.start('hotloop=1')
> > +
> > +-- luacheck: ignore
> > +local anchor
> > +local N_ITER = 5
> > +local SIDE_ITER = N_ITER - 1
> > +for i = 1, N_ITER do
> > +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> > +  -- stack slot, `maxslot` value was unconditionally set to the
> > +  -- destination slot, so the following snapshot is used:
> > +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> > +  -- instead of:
> > +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> Snapshot examples here ceratinly give the idea of what goes wrong,
> but the `0009` and `0001` are meaningless by themselves. I think it would be
> nice to include IRs here too.

Added.

> > +  -- Since these slots are omitted, they are not restored
> > +  -- correctly, when restoring from snapshot for this side exit.
> Please fix the same typos as in the commit message here.

Fixed.

> > +  anchor = ...
> > +  if i > SIDE_ITER then
> > +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> I think better phrasing would be:
> | `test:ok()` is not used here ...

Fixed.

> > +    -- tests in case of `i` incorrect restoring from the snapshot.
> Typo: s/restoring/restoration/

Fixed.

> > +    assert(i > SIDE_ITER)
> > +  end
> > +end
> > +
> > +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
> > +
> > +-- Now the same case, but with an additional frame, so VARG slots
> > +-- are defined on the trace.
> > +local function varg_frame(anchor, i, side_iter, ...)
> > +  anchor = ...
> > +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> > +  -- stack slot, `maxslot` value was unconditionally set to the
> > +  -- destination slot, so the following snapshot is used:
> > +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> > +  -- instead of:
> > +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> > +  -- Since these slots are omitted, they are not restored
> > +  -- correctly, when restoring from snapshot for this side exit.
> I guess we don't need to repeat the entire comment again.

Dropped then.

> > +  if i > side_iter then
> > +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> > +    -- tests in case of `i` incorrect restoring from the snapshot.
> Same typos as in the comment above.

Fixed.

> > +    assert(i > side_iter)

See the iterative patch below. Branch is force-pushed.

===================================================================
diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
index 34c5c572..30a87e54 100644
--- a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
+++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
@@ -12,18 +12,39 @@ local anchor
 local N_ITER = 5
 local SIDE_ITER = N_ITER - 1
 for i = 1, N_ITER do
-  -- In case, when `BC_VARG` set the VARG slot to the non-top
+  -- This trace generates the following IRs:
+  -- 0001 >  int SLOAD  #7    CRI
+  -- 0002 >  int LE     0001  +2147483646
+  -- 0003    int SLOAD  #6    CI
+  -- 0004    int SLOAD  #0    FR
+  -- 0005 >  int LE     0004  +11
+  -- 0006 >  num SLOAD  #5    T
+  -- 0007    num CONV   0003  num.int
+  -- ....        SNAP   #1   [ ---- ---- ---- nil  ]
+  -- 0008 >  num ULE    0007  0006
+  -- 0009  + int ADD    0003  +1
+  -- ....        SNAP   #2   [ ---- ---- ---- nil  ---- ---- ]
+  -- 0010 >  int LE     0009  0001
+  -- ....        SNAP   #3   [ ---- ---- ---- nil  ---- ---- 0009 0001 ---- 0009 ]
+  -- 0011 ------ LOOP ------------
+  -- 0012    num CONV   0009  num.int
+  -- ....        SNAP   #4   [ ---- ---- ---- nil  ]
+  --
+  -- In case, when `BC_VARG` sets the VARG slot to the non-top
   -- stack slot, `maxslot` value was unconditionally set to the
-  -- destination slot, so the following snapshot is used:
-  -- SNAP   #4   [ ---- ---- ---- nil  ]
+  -- destination slot, so the following snapshot (same for the #1)
+  -- is used:
+  -- ....        SNAP   #4   [ ---- ---- ---- nil  ]
   -- instead of:
-  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
+  -- ....        SNAP   #4   [ ---- ---- ---- nil  ---- ---- 0009 0001 ---- 0009 ]
   -- Since these slots are omitted, they are not restored
-  -- correctly, when restoring from snapshot for this side exit.
+  -- correctly, when restoring from the snapshot for this side
+  -- exit.
   anchor = ...
   if i > SIDE_ITER then
-    -- XXX: Don't use `test:ok()` here to avoid double-running of
-    -- tests in case of `i` incorrect restoring from the snapshot.
+    -- XXX: `test:ok()` isn't used here to avoid double-running of
+    -- tests in case of `i` incorrect restoration from the
+    -- snapshot.
     assert(i > SIDE_ITER)
   end
 end
@@ -34,17 +55,10 @@ test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
 -- are defined on the trace.
 local function varg_frame(anchor, i, side_iter, ...)
   anchor = ...
-  -- In case, when `BC_VARG` set the VARG slot to the non-top
-  -- stack slot, `maxslot` value was unconditionally set to the
-  -- destination slot, so the following snapshot is used:
-  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
-  -- instead of:
-  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
-  -- Since these slots are omitted, they are not restored
-  -- correctly, when restoring from snapshot for this side exit.
   if i > side_iter then
-    -- XXX: Don't use `test:ok()` here to avoid double-running of
-    -- tests in case of `i` incorrect restoring from the snapshot.
+    -- XXX: `test:ok()` isn't used here to avoid double-running of
+    -- tests in case of `i` incorrect restoration from the
+    -- snapshot.
     assert(i > side_iter)
   end
 end
===================================================================

<snipped>

> > 

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!
LGTM
On Wed, Aug 16, 2023 at 07:11:57PM +0300, Sergey Kaplun wrote:
> Hi, Maxim!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
> 
> On 16.08.23, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please consider my comments below.
> > 
> > On Tue, Aug 15, 2023 at 03:32:15PM +0300, Sergey Kaplun wrote:
> > > From: Mike Pall <mike>
> > > 
> > > Thanks to Peter Cawley.
> > > 
> > > (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
> > > 
> > > In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
> > Typo: s/set/sets/
> 
> Fixed.
> 
> > > `maxslot` value was unconditionally set to the destination slot, so some
> > > top slots may be omitted in the snapshot entry. Since these slots are
> > > omitted, they are not restored correctly, when restoring from snapshot
> > Typo: s/snapshot/a snapshot/
> 
> Fixed.
> 
> > > for this side exit.
> > > 
> > > This patch adds the check for the aforementioned case, to avoid maxslot
> > > shrinking.
> > > 
> > > Sergey Kaplun:
> > > * added the description and the test for the problem
> > > 
> > > Part of tarantool/tarantool#8825
> > > ---
> > > 
> > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> > > PR: https://github.com/tarantool/tarantool/pull/8986
> > > Related issues:
> > > * https://github.com/LuaJIT/LuaJIT/issues/1046
> > > * https://github.com/tarantool/tarantool/issues/8825
> > > 
> > >  src/lj_record.c                               | 12 +++-
> > >  .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
> > >  2 files changed, 67 insertions(+), 3 deletions(-)
> > >  create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > > 
> > > diff --git a/src/lj_record.c b/src/lj_record.c
> > > index 34d1210a..6bcdb04c 100644
> > > --- a/src/lj_record.c
> > > +++ b/src/lj_record.c
> 
> <snipped>
> 
> > > diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > > new file mode 100644
> > > index 00000000..34c5c572
> > > --- /dev/null
> > > +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> > > @@ -0,0 +1,58 @@
> > > +local tap = require('tap')
> > > +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
> > > +  ['Test requires JIT enabled'] = not jit.status(),
> > > +})
> > > +
> > > +test:plan(2)
> > > +
> > > +jit.opt.start('hotloop=1')
> > > +
> > > +-- luacheck: ignore
> > > +local anchor
> > > +local N_ITER = 5
> > > +local SIDE_ITER = N_ITER - 1
> > > +for i = 1, N_ITER do
> > > +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> > > +  -- stack slot, `maxslot` value was unconditionally set to the
> > > +  -- destination slot, so the following snapshot is used:
> > > +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> > > +  -- instead of:
> > > +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> > Snapshot examples here ceratinly give the idea of what goes wrong,
> > but the `0009` and `0001` are meaningless by themselves. I think it would be
> > nice to include IRs here too.
> 
> Added.
> 
> > > +  -- Since these slots are omitted, they are not restored
> > > +  -- correctly, when restoring from snapshot for this side exit.
> > Please fix the same typos as in the commit message here.
> 
> Fixed.
> 
> > > +  anchor = ...
> > > +  if i > SIDE_ITER then
> > > +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> > I think better phrasing would be:
> > | `test:ok()` is not used here ...
> 
> Fixed.
> 
> > > +    -- tests in case of `i` incorrect restoring from the snapshot.
> > Typo: s/restoring/restoration/
> 
> Fixed.
> 
> > > +    assert(i > SIDE_ITER)
> > > +  end
> > > +end
> > > +
> > > +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
> > > +
> > > +-- Now the same case, but with an additional frame, so VARG slots
> > > +-- are defined on the trace.
> > > +local function varg_frame(anchor, i, side_iter, ...)
> > > +  anchor = ...
> > > +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> > > +  -- stack slot, `maxslot` value was unconditionally set to the
> > > +  -- destination slot, so the following snapshot is used:
> > > +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> > > +  -- instead of:
> > > +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> > > +  -- Since these slots are omitted, they are not restored
> > > +  -- correctly, when restoring from snapshot for this side exit.
> > I guess we don't need to repeat the entire comment again.
> 
> Dropped then.
> 
> > > +  if i > side_iter then
> > > +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> > > +    -- tests in case of `i` incorrect restoring from the snapshot.
> > Same typos as in the comment above.
> 
> Fixed.
> 
> > > +    assert(i > side_iter)
> 
> See the iterative patch below. Branch is force-pushed.
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> index 34c5c572..30a87e54 100644
> --- a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> @@ -12,18 +12,39 @@ local anchor
>  local N_ITER = 5
>  local SIDE_ITER = N_ITER - 1
>  for i = 1, N_ITER do
> -  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- This trace generates the following IRs:
> +  -- 0001 >  int SLOAD  #7    CRI
> +  -- 0002 >  int LE     0001  +2147483646
> +  -- 0003    int SLOAD  #6    CI
> +  -- 0004    int SLOAD  #0    FR
> +  -- 0005 >  int LE     0004  +11
> +  -- 0006 >  num SLOAD  #5    T
> +  -- 0007    num CONV   0003  num.int
> +  -- ....        SNAP   #1   [ ---- ---- ---- nil  ]
> +  -- 0008 >  num ULE    0007  0006
> +  -- 0009  + int ADD    0003  +1
> +  -- ....        SNAP   #2   [ ---- ---- ---- nil  ---- ---- ]
> +  -- 0010 >  int LE     0009  0001
> +  -- ....        SNAP   #3   [ ---- ---- ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> +  -- 0011 ------ LOOP ------------
> +  -- 0012    num CONV   0009  num.int
> +  -- ....        SNAP   #4   [ ---- ---- ---- nil  ]
> +  --
> +  -- In case, when `BC_VARG` sets the VARG slot to the non-top
>    -- stack slot, `maxslot` value was unconditionally set to the
> -  -- destination slot, so the following snapshot is used:
> -  -- SNAP   #4   [ ---- ---- ---- nil  ]
> +  -- destination slot, so the following snapshot (same for the #1)
> +  -- is used:
> +  -- ....        SNAP   #4   [ ---- ---- ---- nil  ]
>    -- instead of:
> -  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> +  -- ....        SNAP   #4   [ ---- ---- ---- nil  ---- ---- 0009 0001 ---- 0009 ]
>    -- Since these slots are omitted, they are not restored
> -  -- correctly, when restoring from snapshot for this side exit.
> +  -- correctly, when restoring from the snapshot for this side
> +  -- exit.
>    anchor = ...
>    if i > SIDE_ITER then
> -    -- XXX: Don't use `test:ok()` here to avoid double-running of
> -    -- tests in case of `i` incorrect restoring from the snapshot.
> +    -- XXX: `test:ok()` isn't used here to avoid double-running of
> +    -- tests in case of `i` incorrect restoration from the
> +    -- snapshot.
>      assert(i > SIDE_ITER)
>    end
>  end
> @@ -34,17 +55,10 @@ test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
>  -- are defined on the trace.
>  local function varg_frame(anchor, i, side_iter, ...)
>    anchor = ...
> -  -- In case, when `BC_VARG` set the VARG slot to the non-top
> -  -- stack slot, `maxslot` value was unconditionally set to the
> -  -- destination slot, so the following snapshot is used:
> -  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> -  -- instead of:
> -  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> -  -- Since these slots are omitted, they are not restored
> -  -- correctly, when restoring from snapshot for this side exit.
>    if i > side_iter then
> -    -- XXX: Don't use `test:ok()` here to avoid double-running of
> -    -- tests in case of `i` incorrect restoring from the snapshot.
> +    -- XXX: `test:ok()` isn't used here to avoid double-running of
> +    -- tests in case of `i` incorrect restoration from the
> +    -- snapshot.
>      assert(i > side_iter)
>    end
>  end
> ===================================================================
> 
> <snipped>
> 
> > > 
> 
> -- 
> Best regards,
> Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for the patch! See comment inline.


On 8/15/23 15:32, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
>
> In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
> `maxslot` value was unconditionally set to the destination slot, so some
> top slots may be omitted in the snapshot entry. Since these slots are
> omitted, they are not restored correctly, when restoring from snapshot
> for this side exit.
>
> This patch adds the check for the aforementioned case, to avoid maxslot
> shrinking.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#8825
> ---
>
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> PR: https://github.com/tarantool/tarantool/pull/8986
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1046
> * https://github.com/tarantool/tarantool/issues/8825
>
>   src/lj_record.c                               | 12 +++-
>   .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
>   2 files changed, 67 insertions(+), 3 deletions(-)
>   create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
>
> diff --git a/src/lj_record.c b/src/lj_record.c
> index 34d1210a..6bcdb04c 100644
> --- a/src/lj_record.c
> +++ b/src/lj_record.c
> @@ -1807,8 +1807,12 @@ 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)nresults;
> +    if (nresults != 1) {
> +      if (nresults == -1) nresults = nvararg;
> +      J->maxslot = dst + (BCReg)nresults;
> +    } else if (dst >= J->maxslot) {
> +      J->maxslot = dst + 1;
> +    }
>       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. */
> @@ -1840,7 +1844,9 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
>         }
>         for (i = nvararg; i < nresults; i++)
>   	J->base[dst+i] = TREF_NIL;
> -      J->maxslot = dst + (BCReg)nresults;
> +      if (nresults != 1 || dst >= J->maxslot) {
> +	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-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> new file mode 100644
> index 00000000..34c5c572
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> @@ -0,0 +1,58 @@
> +local tap = require('tap')
> +local test = tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(2)
> +
> +jit.opt.start('hotloop=1')
> +
> +-- luacheck: ignore
> +local anchor
> +local N_ITER = 5
> +local SIDE_ITER = N_ITER - 1
> +for i = 1, N_ITER do
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> +  -- instead of:
> +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
> +  anchor = ...
> +  if i > SIDE_ITER then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> +    -- tests in case of `i` incorrect restoring from the snapshot.
> +    assert(i > SIDE_ITER)
> +  end
> +end
> +
> +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')

This always passed testcase is confused and it will confuse everyone who 
will use test.

I propose to replace test:ok() to test:diag:

diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua 
b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
index 30a87e54..32566c54 100644
--- a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
+++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
@@ -3,7 +3,7 @@ local test = 
tap.test('lj-1046-fix-bc-varg-recording'):skipcond({
    ['Test requires JIT enabled'] = not jit.status(),
  })

-test:plan(2)
+test:plan(0)

  jit.opt.start('hotloop=1')

@@ -49,7 +49,7 @@ for i = 1, N_ITER do
    end
  end

-test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
+test:diag('BC_VARG recording 0th frame depth, 1 result')

  -- Now the same case, but with an additional frame, so VARG slots
  -- are defined on the trace.
@@ -67,6 +67,6 @@ for i = 1, N_ITER do
    varg_frame(nil, i, SIDE_ITER)
  end

-test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 
result')
+test:diag('BC_VARG recording with VARG slots defined on trace, 1 result')

  test:done(true)


or you can define a global variable and replace assert with assigning 
result to this variable.

then check expected and actual value for global variable using test:ok().

something like this:


  test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')

+local varg_recorded = false
+
  -- Now the same case, but with an additional frame, so VARG slots
  -- are defined on the trace.
  local function varg_frame(anchor, i, side_iter, ...)
    anchor = ...
-  if i > side_iter then
-    -- XXX: `test:ok()` isn't used here to avoid double-running of
-    -- tests in case of `i` incorrect restoration from the
-    -- snapshot.
-    assert(i > side_iter)
-  end
+  varg_recorded = i > side_iter
  end

  for i = 1, N_ITER do
    varg_frame(nil, i, SIDE_ITER)
  end

-test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 
result')
+test:ok(varg_recorded, 'BC_VARG recording with VARG slots defined on 
trace, 1 result')

  test:done(true)


> +
> +-- Now the same case, but with an additional frame, so VARG slots
> +-- are defined on the trace.
> +local function varg_frame(anchor, i, side_iter, ...)
> +  anchor = ...
> +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> +  -- stack slot, `maxslot` value was unconditionally set to the
> +  -- destination slot, so the following snapshot is used:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil ]
> +  -- instead of:
> +  -- SNAP   #4   [ <snipped> | nil  nil  nil  `varg_frame` | nil 0009 0005 ]
> +  -- Since these slots are omitted, they are not restored
> +  -- correctly, when restoring from snapshot for this side exit.
> +  if i > side_iter then
> +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> +    -- tests in case of `i` incorrect restoring from the snapshot.
> +    assert(i > side_iter)
> +  end
> +end
> +
> +for i = 1, N_ITER do
> +  varg_frame(nil, i, SIDE_ITER)
> +end
> +
> +test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result')
> +
> +test:done(true)

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the review!
Fixed your comment and force-pushed the branch.

On 21.08.23, Sergey Bronnikov wrote:
> Hi, Sergey
> 
> thanks for the patch! See comment inline.
> 
> 
> On 8/15/23 15:32, Sergey Kaplun wrote:
> > From: Mike Pall <mike>

<snipped>

> > +local anchor
> > +local N_ITER = 5
> > +local SIDE_ITER = N_ITER - 1
> > +for i = 1, N_ITER do
> > +  -- In case, when `BC_VARG` set the VARG slot to the non-top
> > +  -- stack slot, `maxslot` value was unconditionally set to the
> > +  -- destination slot, so the following snapshot is used:
> > +  -- SNAP   #4   [ ---- ---- ---- nil  ]
> > +  -- instead of:
> > +  -- SNAP   #4   [ ---- nil  ---- ---- 0009 0001 ---- 0009 ]
> > +  -- Since these slots are omitted, they are not restored
> > +  -- correctly, when restoring from snapshot for this side exit.
> > +  anchor = ...
> > +  if i > SIDE_ITER then
> > +    -- XXX: Don't use `test:ok()` here to avoid double-running of
> > +    -- tests in case of `i` incorrect restoring from the snapshot.
> > +    assert(i > SIDE_ITER)
> > +  end
> > +end
> > +
> > +test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
> 
> This always passed testcase is confused and it will confuse everyone who 
> will use test.
> 
> I propose to replace test:ok() to test:diag:
> 

<snipped>

> 
> 
> or you can define a global variable and replace assert with assigning 
> result to this variable.

I preferred this option:

===================================================================
diff --git a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
index 30a87e54..8bdd49eb 100644
--- a/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
+++ b/test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
@@ -11,6 +11,9 @@ jit.opt.start('hotloop=1')
 local anchor
 local N_ITER = 5
 local SIDE_ITER = N_ITER - 1
+
+local consistent_snap_restore = false
+
 for i = 1, N_ITER do
   -- This trace generates the following IRs:
   -- 0001 >  int SLOAD  #7    CRI
@@ -45,11 +48,12 @@ for i = 1, N_ITER do
     -- XXX: `test:ok()` isn't used here to avoid double-running of
     -- tests in case of `i` incorrect restoration from the
     -- snapshot.
-    assert(i > SIDE_ITER)
+    consistent_snap_restore = i > SIDE_ITER
+    break
   end
 end
 
-test:ok(true, 'BC_VARG recording 0th frame depth, 1 result')
+test:ok(consistent_snap_restore, 'BC_VARG recording 0th frame depth, 1 result')
 
 -- Now the same case, but with an additional frame, so VARG slots
 -- are defined on the trace.
@@ -59,14 +63,17 @@ local function varg_frame(anchor, i, side_iter, ...)
     -- XXX: `test:ok()` isn't used here to avoid double-running of
     -- tests in case of `i` incorrect restoration from the
     -- snapshot.
-    assert(i > side_iter)
+    consistent_snap_restore = i > side_iter
   end
 end
 
+consistent_snap_restore = false
+
 for i = 1, N_ITER do
   varg_frame(nil, i, SIDE_ITER)
 end
 
-test:ok(true, 'BC_VARG recording with VARG slots defined on trace, 1 result')
+test:ok(consistent_snap_restore,
+        'BC_VARG recording with VARG slots defined on trace, 1 result')
 
 test:done(true)
===================================================================

> 
> then check expected and actual value for global variable using test:ok().
> 

<snipped>

> 
> 
> > +

<snipped>


-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey


thanks for your fixes proposed in [1], I like your suggested changes.

LGTM after fixing red CI, see in [2].


1. https://lists.tarantool.org/tarantool-patches/ZOTXyLt2x9FcXCt6@root/

2. 
https://github.com/tarantool/luajit/actions/runs/5950318637/job/16138150847


On 8/15/23 15:32, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
>
> In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
> `maxslot` value was unconditionally set to the destination slot, so some
> top slots may be omitted in the snapshot entry. Since these slots are
> omitted, they are not restored correctly, when restoring from snapshot
> for this side exit.
>
> This patch adds the check for the aforementioned case, to avoid maxslot
> shrinking.
>

<snipped>

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

[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 15.08.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry-picked from commit abb27c7771947e082c9d919d184ad5f5f03e2e32)
> 
> In case, when `BC_VARG` set the VARG slot to the non-top stack slot,
> `maxslot` value was unconditionally set to the destination slot, so some
> top slots may be omitted in the snapshot entry. Since these slots are
> omitted, they are not restored correctly, when restoring from snapshot
> for this side exit.
> 
> This patch adds the check for the aforementioned case, to avoid maxslot
> shrinking.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8825
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1046-fix-bc-varg-recording
> PR: https://github.com/tarantool/tarantool/pull/8986
> Related issues:
> * https://github.com/LuaJIT/LuaJIT/issues/1046
> * https://github.com/tarantool/tarantool/issues/8825
> 
>  src/lj_record.c                               | 12 +++-
>  .../lj-1046-fix-bc-varg-recording.test.lua    | 58 +++++++++++++++++++
>  2 files changed, 67 insertions(+), 3 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-1046-fix-bc-varg-recording.test.lua
> 

<snipped>

> -- 
> 2.41.0
> 

-- 
Best regards,
IM