From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 506896ECE3; Fri, 15 Jul 2022 18:03:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 506896ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1657897430; bh=JwnLeWTxplDKApG1JnAZ/XXQaKjN1h3/FXIx94a0UGQ=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=WvfZnCmEFpSbu6W1TRmr9KhXl951ucmfJZqCh7zvsF3KcRSwbapx8zWps8OaYpEGW vXqoUnVTJ8VfT2WkPAKWkdP9B4lnJ0PE7hLoyRd3WjVurIs1HR6vogn35eLqq3h88a ofYhb/umkS4ajErmYIPIRRrUE99TZti+PQcIRHfo= Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 12A106ECE3 for ; Fri, 15 Jul 2022 18:03:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 12A106ECE3 Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1oCMr6-00070U-A5; Fri, 15 Jul 2022 18:03:48 +0300 Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_565E2137-0F32-4D97-9A62-39F01B6A40B8" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.100.31\)) Date: Fri, 15 Jul 2022 18:03:47 +0300 In-Reply-To: To: Sergey Kaplun References: <20220713095349.31718-1-skaplun@tarantool.org> <8B86F311-0882-4D94-9FD4-EB93EB11393C@tarantool.org> X-Mailer: Apple Mail (2.3696.100.31) X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD97CF746166DCF1A147F1AC4B79E5AAD994CD68C6A3DE455E7182A05F538085040F84CEA6EE7523E7F223984099323F039A6D07B938442A8CD951FA000EDE267C5 X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B01A5684911C72D27BD71D610BA8056A101195D4291B9DFA3AB43E4C17E1D7DCB69C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C5CF6F4B28551E3F16E27EF1014AE20E9278559AEADBAF850DD52B28C3B124A7DF2D31E67D384DA21D7E09C32AA3244CFB859AA204BE6F98536F79E93F4A37F305AB220A9D022EBCFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojpf3/rFe+bF+gSPDKvQLUNw== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407CD794FDA45297ABD562341C93CACE3C3258B2A4AE681343219381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" --Apple-Mail=_565E2137-0F32-4D97-9A62-39F01B6A40B8 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thanks, LGTM. Sergos > On 15 Jul 2022, at 17:44, Sergey Kaplun wrote: >=20 > Hi, Sergos! >=20 > Thanks for the review! >=20 > On 14.07.22, sergos wrote: >> Hi! >>=20 >> Thanks for the patch, some updates to the test. >>=20 >> Regards, >> Sergos >>=20 >>> On 13 Jul 2022, at 12:53, Sergey Kaplun = wrote: >>>=20 >>> From: Mike Pall >>>=20 >>> Reported by George Vaintrub. Fixed by Sergey Kaplun. >>>=20 >>> (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1) >>>=20 >>> 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=E2=80=99 one? >=20 > 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. >=20 >>=20 >>> 2) known fixed number of results. >>>=20 >>> For this case the vararg slots loads via `IR_VLOAD` by offset from >> are loaded by using an >>=20 >>> vararg base. In GC64 mode this offset was miscounting due to missing >> the the is miscounted >>=20 >>> `LJ_FR2` correction in the base TRef calculation. As the result the >>> wrong (+1) vararg slot is used. >>>=20 >>> This patch adds the missing the aforementioned `LJ_FR2` correction. >> xxx >=20 > I've updated commit message to the following: > Branch is force-pushed. >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > LJ_GC64: Fix IR_VARG offset for fixed number of results. >=20 > Reported by George Vaintrub. Fixed by Sergey Kaplun. >=20 > (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1) >=20 > 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. >=20 > 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. >=20 > This patch adds the missing aforementioned `LJ_FR2` correction. >=20 > Sergey Kaplun: > * added the description and the test for the problem >=20 > Resolves tarantool/tarantool#7172 > Part of tarantool/tarantool#7230 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 >>>=20 >>> Sergey Kaplun: >>> * added the description and the test for the problem >>>=20 >>> Resolves tarantool/tarantool#7172 >>> Part of tarantool/tarantool#7230 >>> --- >>>=20 >>> Branch: = https://github.com/tarantool/luajit/tree/skaplun/lj-864-varg-rec-base-offs= et-full-ci >>> Issues: >>> * https://github.com/tarantool/tarantool/issues/7172 >>> * https://github.com/LuaJIT/LuaJIT/issues/864 >>>=20 >>> 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 >>>=20 >>> 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 =3D emitir(IRT(IR_SUB, IRT_IGC), REF_BASE, fr); >>> - vbase =3D emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, = frofs-8)); >>> + vbase =3D emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, = frofs-8*(1+LJ_FR2))); >> Wherearemyspaces? >> (nevermind, just a moan) >=20 > Yes, but this style is preferred by Mike (see code below) :). >=20 >>=20 >>> for (i =3D 0; i < nload; i++) { >>> IRType t =3D itype2irt(&J->L->base[i-1-LJ_FR2-nvararg]); >>> TRef aref =3D 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 =3D require('tap') >>> + >>> +-- Test file to demonstrate LuaJIT misbehaviour during recording >>> +-- BC_VARG with nvarargs >=3D nresults in GC64 mode. >> In the message you say it should be unknown. What=E2=80=99s the dirty = truth is? >>=20 >>> +-- See also https://github.com/LuaJIT/LuaJIT/issues/864, >>> +-- https://github.com/tarantool/tarantool/issues/7172. >>> +local test =3D tap.test('lj-864-varg-rec-base-offset') >>> +test:plan(1) >>> + >>> +jit.opt.start('hotloop=3D1') >>> + >>> +local MAGIC =3D 42 >> Should be enough to test against the first argument, no MAGIC :) >>=20 >>> +local function test_rec_varg(...) >>> + local slot1 >>> + for _ =3D 1, 3 do >>> + slot1 =3D ... >>> + end >> ++ args =3D {...} >> +- return slot1 =3D=3D args[1] >>> +end >=20 > I've rewritten test wo MAGIC :). >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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) >=20 > jit.opt.start('hotloop=3D1') >=20 > -local MAGIC =3D 42 > local function test_rec_varg(...) > - local slot1 > + local trace_value, interp_value > for _ =3D 1, 3 do > - slot1 =3D ... > + trace_value =3D ... > end > - return slot1 =3D=3D MAGIC > + interp_value =3D ... > + return trace_value =3D=3D interp_value > end >=20 > -- Test case for nvarargs >=3D 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') >=20 > os.exit(test:check() and 0 or 1) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 >>> + >>> +-- Test case for nvarargs >=3D 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) >>> --=20 >>> 2.34.1 >>>=20 >>=20 >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_565E2137-0F32-4D97-9A62-39F01B6A40B8 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi!

Thanks,= LGTM.

Sergos


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=E2=80=99 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.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


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-re= c-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 =3D = emitir(IRT(IR_SUB, IRT_IGC), REF_BASE, fr);
- vbase =3D = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8));
+ = vbase =3D 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 =3D = 0; i < nload; i++) {
 IRType t =3D = itype2irt(&J->L->base[i-1-LJ_FR2-nvararg]);
 TRef aref =3D = 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 =3D = require('tap')
+
+-- Test file to = demonstrate LuaJIT misbehaviour during recording
+-- = BC_VARG with nvarargs >=3D nresults in GC64 mode.
In the message you say it should be unknown. = What=E2=80=99s the dirty truth is?

+-- See also https://github.com/LuaJIT/LuaJIT/issues/864,
+-- https://github.com/tarantool/tarantool/issues/7172.
+local test =3D tap.test('lj-864-varg-rec-base-offset')
+test:plan(1)
+
+jit.opt.start('hotloop=3D1')
+
+local MAGIC =3D 42
Should be = enough to test against the first argument, no MAGIC :)

+local function = test_rec_varg(...)
+ local slot1
+ for _ =3D = 1, 3 do
+ slot1 =3D ...
+ end
++ args =3D {...}
+- return slot1 = =3D=3D args[1]
+end

I've rewritten test wo MAGIC :).

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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=3D1')

-local MAGIC =3D 42
local function test_rec_varg(...)
- local = slot1
+ local = trace_value, interp_value
for _ =3D 1, 3 do
- slot1 =3D ...
+ trace_value =3D ...
end
- return slot1 =3D=3D MAGIC
+ interp_value =3D ...
+ return trace_value =3D=3D interp_value
end

-- Test case = for nvarargs >=3D 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)
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

+
+-- Test = case for nvarargs >=3D 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

= --Apple-Mail=_565E2137-0F32-4D97-9A62-39F01B6A40B8--