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

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

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