Hi! Thanks, LGTM. Sergos > On 15 Jul 2022, at 17:44, Sergey Kaplun wrote: > > 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 wrote: >>> >>> From: Mike Pall >>> >>> 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