[Tarantool-patches] [PATCH luajit] Fix maxslots when recording BC_VARG, part 3.
Sergey Kaplun
skaplun at tarantool.org
Wed Aug 16 19:11:57 MSK 2023
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
More information about the Tarantool-patches
mailing list