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

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

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