On 15 Jul 2022, at 17:44, Sergey Kaplun <skaplun@tarantool.org> 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 <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