Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results.

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

Hi, Sergos!

Thanks for the review!

On 14.07.22, sergos wrote:
> Hi!
> 
> Thanks for the patch, some updates to the test.
> 
> Regards,
> Sergos
> 
> > On 13 Jul 2022, at 12:53, Sergey Kaplun <skaplun@tarantool.org> wrote:
> > 
> > From: Mike Pall <mike>
> > 
> > Reported by George Vaintrub. Fixed by Sergey Kaplun.
> > 
> > (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1)
> > 
> > This bug occurs when recording `BC_VARG` with the following conditions:
> > 1) varargs undefined on trace.
> Later in the test you just mention its size should be bigger than results’ one?

I meant that the __content__ of these slots is undefined, because the trace
is started inside current function and we don't know the caller's context.
So we can't just copy these slots.

> 
> > 2) known fixed number of results.
> > 
> > For this case the vararg slots loads via `IR_VLOAD` by offset from
>                                 are loaded by       using an
> 
> > vararg base. In GC64 mode this offset was miscounting due to missing
> the             the                      is miscounted
> 
> > `LJ_FR2` correction in the base TRef calculation. As the result the
> > wrong (+1) vararg slot is used.
> > 
> > This patch adds the missing the aforementioned `LJ_FR2` correction.
>                               xxx

I've updated commit message to the following:
Branch is force-pushed.

===================================================================
LJ_GC64: Fix IR_VARG offset for fixed number of results.

Reported by George Vaintrub. Fixed by Sergey Kaplun.

(cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1)

This bug occurs when recording `BC_VARG` with the following conditions:
1) A trace is started inside current vararg function, so the content of
   varargs slots is undefined for the trace.
2) Known fixed number of results.

For this case the vararg slots are loaded by `IR_VLOAD` using an offset
from the vararg base. In the GC64 mode this offset is miscounted due to
missing `LJ_FR2` correction in the base TRef calculation. As the result
the wrong (+1) vararg slot is used.

This patch adds the missing aforementioned `LJ_FR2` correction.

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

Resolves tarantool/tarantool#7172
Part of tarantool/tarantool#7230
===================================================================

> > 
> > Sergey Kaplun:
> > * added the description and the test for the problem
> > 
> > Resolves tarantool/tarantool#7172
> > Part of tarantool/tarantool#7230
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-864-varg-rec-base-offset-full-ci
> > Issues:
> > * https://github.com/tarantool/tarantool/issues/7172
> > * https://github.com/LuaJIT/LuaJIT/issues/864
> > 
> > src/lj_record.c                               |  2 +-
> > .../lj-864-varg-rec-base-offset.test.lua      | 25 +++++++++++++++++++
> > 2 files changed, 26 insertions(+), 1 deletion(-)
> > create mode 100644 test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> > 
> > diff --git a/src/lj_record.c b/src/lj_record.c
> > index a11f3712..9e2e1d9e 100644
> > --- a/src/lj_record.c
> > +++ b/src/lj_record.c
> > @@ -1794,7 +1794,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults)
> > 	  emitir(IRTGI(IR_EQ), fr,
> > 		 lj_ir_kint(J, (int32_t)frame_ftsz(J->L->base-1)));
> > 	vbase = emitir(IRT(IR_SUB, IRT_IGC), REF_BASE, fr);
> > -	vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8));
> > +	vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8*(1+LJ_FR2)));
>                                                                          Wherearemyspaces?
>                                                                       (nevermind, just a moan)

Yes, but this style is preferred by Mike (see code below) :).

> 
> > 	for (i = 0; i < nload; i++) {
> > 	  IRType t = itype2irt(&J->L->base[i-1-LJ_FR2-nvararg]);
> > 	  TRef aref = emitir(IRT(IR_AREF, IRT_PGC),
> > diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> > new file mode 100644
> > index 00000000..ca30f92f
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
> > @@ -0,0 +1,25 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT misbehaviour during recording
> > +-- BC_VARG with nvarargs >= nresults in GC64 mode.
> In the message you say it should be unknown. What’s the dirty truth is?
> 
> > +-- See also https://github.com/LuaJIT/LuaJIT/issues/864,
> > +-- https://github.com/tarantool/tarantool/issues/7172.
> > +local test = tap.test('lj-864-varg-rec-base-offset')
> > +test:plan(1)
> > +
> > +jit.opt.start('hotloop=1')
> > +
> > +local MAGIC = 42
> Should be enough to test against the first argument, no MAGIC :)
> 
> > +local function test_rec_varg(...)
> > +  local slot1
> > +  for _ = 1, 3 do
> > +    slot1 = ...
> > +  end
> ++  args = {...}
> +-  return slot1 == args[1]
> > +end

I've rewritten test wo MAGIC :).

===================================================================
diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
index ca30f92f..d74c3c2b 100644
--- a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
+++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua
@@ -9,17 +9,17 @@ test:plan(1)
 
 jit.opt.start('hotloop=1')
 
-local MAGIC = 42
 local function test_rec_varg(...)
-  local slot1
+  local trace_value, interp_value
   for _ = 1, 3 do
-    slot1 = ...
+    trace_value = ...
   end
-  return slot1 == MAGIC
+  interp_value = ...
+  return trace_value == interp_value
 end
 
 -- Test case for nvarargs >= nresults. Equality is not suitable
 -- due to failing assertion guard for type of loaded vararg slot.
-test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording')
+test:ok(test_rec_varg(42, 0), 'correct BC_VARG recording')
 
 os.exit(test:check() and 0 or 1)
===================================================================

> > +
> > +-- Test case for nvarargs >= nresults. Equality is not suitable
> > +-- due to failing assertion guard for type of loaded vararg slot.
> > +test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording')
> > +
> > +os.exit(test:check() and 0 or 1)
> > -- 
> > 2.34.1
> > 
> 

-- 
Best regards,
Sergey Kaplun