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 03F486E0CA; Mon, 10 Jan 2022 14:48:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 03F486E0CA DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1641815323; bh=fgWZ6ZiWPiOAbvyqLWyBfdCYXyRKMwrMhl8Zujc5Ofc=; 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=nq2MSWmfZDcwAUpxVXWAlqNINsJpYuCyu0jqwjAENalXpFf5BF71skD4pqRq9KtvK nREvs5rkCHTzglRzyQlpMQa4MMmHwtGs9h4b5KyJewkN7O1OYOef3un69SRPyfZ3nF 1XwoutCDDtgHjZuNmAK4WNX7XDjhMWo5bQPL/jSk= Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 2595A6E0CA for ; Mon, 10 Jan 2022 14:48:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2595A6E0CA Received: by smtp52.i.mail.ru with esmtpa (envelope-from ) id 1n6tAG-0002mS-9W; Mon, 10 Jan 2022 14:48:40 +0300 Message-Id: <41269333-3FC1-4AD3-979B-7A823BB0D0C9@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_044512A5-617A-447A-A792-27B573104B99" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\)) Date: Mon, 10 Jan 2022 14:48:39 +0300 In-Reply-To: To: Sergey Kaplun References: <20210820154846.5515-1-skaplun@tarantool.org> X-Mailer: Apple Mail (2.3654.120.0.1.13) X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD94D5EF110843E6A6756D129FFADCD37D44B66CFF8B7714B73182A05F53808504018A01D115F977BC8D3F828736DAC1580EC98F5F2D9EB7FA5F64CFFC90E82D007 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F544D30F1A6FA191EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C83F54BD885518138638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8DFF4209093BE83B746CA21C5E0C3DFAF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE41BF15D38FB6CB3ABDB03A3F2A65D472D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE34AB4081B6A6C2E076E0066C2D8992A16C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F790063703AFC36FC312010EEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5BDBF4C61C8D9F6A2A155E621282619368FFB2A0F5A613E82D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA756888F6138A55F47E410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340DAE5B306C240CF593D1AAC15CB3D34189E3C77F3EBAE7C59D821FB57541B251052986EEDFAAF63E1D7E09C32AA3244C28BE30C50C382E37D8E169B0334A0D07E646F07CC2D4F3D8FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojA9AFnsZ9k5Lc7oTIl8lNCA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769AE2B4BE123D4EB99D3F828736DAC1580EFC4EB6826DC224860D8632BEC246C7D55B4A2144138A8805FC805B5969CB4993EE16157CC7DAB4272D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix string.char() recording with no arguments. 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=_044512A5-617A-447A-A792-27B573104B99 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Thanks, LGTM. Sergos > On 1 Sep 2021, at 10:10, Sergey Kaplun wrote: >=20 > Hi, Sergos! >=20 > Thanks for the review! >=20 > On 31.08.21, Sergey Ostanevich wrote: >> Hi! Thanks for the patch! >>=20 >> Some readability comments below. >>=20 >> regards, >> Sergos >>=20 >>=20 >=20 > The new commit message is the following: >=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 > Fix string.char() recording with no arguments. >=20 > (cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a) >=20 > `string.char()` call without arguments yields an empty string. JIT > recording machinery doesn=E2=80=99t handle this case. Each recording = of a fast > function expects 1 result by default. Hence, when return from this = call > is recorded the framelink slot (the top slot value) is considered as a > result to yield. It is loaded into the corresponding slot as an IR = with > `IRT_NUM` type. It leads to assertion failure in `rec_check_slots()`, > when a next bytecode is recorded, because type of TValue on the stack > (`LJ_STR`) isn't the same as IR (and TRef) type. >=20 > This patch handles the case without arguments by the loading of IR = with > empty string reference into the corresponding slot. It reuses = assumption > of one result by default, hence there is no case for `i =3D=3D 1` in = the > code. >=20 > Sergey Kaplun: > * added the description and the test for the problem >=20 > Resolves tarantool/tarantool#6371 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=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 > Branch is force-pushed. >=20 >>> On 20 Aug 2021, at 18:48, Sergey Kaplun > wrote: >>>=20 >>> From: Mike Pall >>>=20 >>> (cherry picked from commit dfa692b746c9de067857d5fc992a41730be3d99a) >>>=20 >>> `string.char()` call without arguments yields an empty string. When = JIT >>> machinery records the aforementioned call it doesn't handle this = case. >> JIT recording machinery doesn=E2=80=99t handle this case. >=20 > Fixed. >=20 >>=20 >>> Each recording fast function expects 1 result by default. Hence, = when >> ^ >> of a=20 >=20 > Fixed. >=20 >>=20 >>> return from this call is recorded the framelink slot is used as a >>> result. It is loaded into the corresponding slot as an IR with = `IRT_NUM` >>=20 >> I have a question here: is this number denotes the number of results? >> Then, perhaps reword the previous sentence that this very number is=20= >> considered as result. >=20 > No, it means that the corresponding stack slot type is a number. But, > the result of the call must be the string, not a number. >=20 >>=20 >>> type. It leads to assertion failure in `rec_check_slots()`, when a = next >>> bytecode is recorded, because type of TValue on the stack (`LJ_STR`) >>> isn't the same as IR (and TRef) type. >>>=20 >>> This patch handles the case without arguments by the loading of IR = with >>> empty string reference into the corresponding slot. >>>=20 >> I would add that code reuses assumption of one result by default, >> hence no case for =E2=80=98i =3D=3D 1=E2=80=99 in the code. >=20 > Added. >=20 >>=20 >>> Sergey Kaplun: >>> * added the description and the test for the problem >>>=20 >>> Resolves tarantool/tarantool#6371 >>> --- >>>=20 >>> Branch: = https://github.com/tarantool/luajit/tree/skaplun/gh-6371-string-char-no-ar= g >>> Issue: https://github.com/tarantool/tarantool/issues/6371 >>> Tarantool branch: = https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-string-char-no= -arg >>> Side note: CI is totally red, but AFAICS it's unrelated with my = patch. >>> Side note: See also Changelog at the Tarantool branch. >>>=20 >>> src/lj_ffrecord.c | 2 ++ >>> .../gh-6371-string-char-no-arg.test.lua | 28 = +++++++++++++++++++ >>> 2 files changed, 30 insertions(+) >>> create mode 100644 = test/tarantool-tests/gh-6371-string-char-no-arg.test.lua >>>=20 >>> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c >>> index 8dfa80ed..be890a93 100644 >>> --- a/src/lj_ffrecord.c >>> +++ b/src/lj_ffrecord.c >>> @@ -866,6 +866,8 @@ static void LJ_FASTCALL = recff_string_char(jit_State *J, RecordFFData *rd) >>> for (i =3D 0; J->base[i] !=3D 0; i++) >>> tr =3D emitir(IRT(IR_BUFPUT, IRT_PGC), tr, J->base[i]); >>> J->base[0] =3D emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr); >>> + } else if (i =3D=3D 0) { >>> + J->base[0] =3D lj_ir_kstr(J, &J2G(J)->strempty); >>> } >>> UNUSED(rd); >>> } >>> diff --git = a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua = b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua >>> new file mode 100644 >>> index 00000000..6df93f07 >>> --- /dev/null >>> +++ b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua >>> @@ -0,0 +1,28 @@ >>> +local tap =3D require('tap') >>> + >>> +-- Test file to demonstrate assertion after `string.char()` >>> +-- recording. >>> +-- See also, https://github.com/tarantool/tarantool/issues/6371. >>> + >>> +local test =3D tap.test('gh-6371-string-char-no-arg') >>> +-- XXX: Number of loop iterations. >>> +-- 1, 2 -- instruction becomes hot >>> +-- 3 -- trace is recorded (considering loop recording specifics), >>> +-- but bytecodes are still executed via VM >>> +-- 4 -- trace is executed, need to check that emitted mcode is >>> +-- correct >>> +local NTEST =3D 4 >>> +test:plan(NTEST) >>> + >>> +-- Storage for the results to avoid trace aborting by `test:ok()`. >>> +local results =3D {} >>> +jit.opt.start('hotloop=3D1') >>> +for _ =3D 1, NTEST do >>> + table.insert(results, string.char()) >>> +end >>> + >>> +for i =3D 1, NTEST do >>> + test:ok(results[i] =3D=3D '', 'correct recording of string.char() = without args') >>> +end >>> + >>> +os.exit(test:check() and 0 or 1) >>> --=20 >>> 2.31.0 >>>=20 >>=20 >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_044512A5-617A-447A-A792-27B573104B99 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Thanks, LGTM.

Sergos

On 1 Sep 2021, at 10:10, Sergey = Kaplun <skaplun@tarantool.org> wrote:

Hi, Sergos!

Thanks for the review!

On 31.08.21, Sergey Ostanevich wrote:
Hi! = Thanks for the patch!

Some readability = comments below.

regards,
Sergos



The new commit message is the following:

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
Fix = string.char() recording with no arguments.

(cherry picked from commit = dfa692b746c9de067857d5fc992a41730be3d99a)

`string.char()` call without arguments yields an empty = string. JIT
recording = machinery doesn=E2=80=99t handle this case. Each recording of a = fast
function = expects 1 result by default. Hence, when return from this call
is recorded = the framelink slot (the top slot value) is considered as a
result to = yield. It is loaded into the corresponding slot as an IR with
`IRT_NUM` = type. It leads to assertion failure in `rec_check_slots()`,
when a next = bytecode is recorded, because type of TValue on the stack
(`LJ_STR`) = isn't the same as IR (and TRef) type.

This patch handles the case without arguments by the loading = of IR with
empty string = reference into the corresponding slot. It reuses assumption
of one result = by default, hence there is no case for `i =3D=3D 1` in the
code.
Sergey = Kaplun:
* added the = description and the test for the problem

Resolves tarantool/tarantool#6371
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Branch is = force-pushed.

On 20 Aug 2021, at = 18:48, Sergey Kaplun <skaplun@tarantool.org> wrote:

From: Mike Pall <mike>

(cherry picked from commit = dfa692b746c9de067857d5fc992a41730be3d99a)

`string.char()` call without arguments yields an empty = string. When JIT
machinery records the aforementioned call = it doesn't handle this case.
JIT recording = machinery doesn=E2=80=99t handle this case.
Fixed.


Each recording fast function expects 1 result = by default.  Hence, when
        &n= bsp;      ^
          &nb= sp;   of a 

Fixed.


return from this call is recorded the framelink = slot is used as a
result. It is loaded into the = corresponding slot as an IR with `IRT_NUM`

I have a question here: is this number denotes the number of = results?
Then, perhaps reword the previous sentence that = this very number is 
considered as result.

No, it means = that the corresponding stack slot type is a number. But,
the result of = the call must be the string, not a number.


type. It leads to assertion failure in = `rec_check_slots()`, when a next
bytecode is recorded, = because type of TValue on the stack (`LJ_STR`)
isn't the = same as IR (and TRef) type.

This patch = handles the case without arguments by the loading of IR with
empty string reference into the corresponding slot.

I would add that code reuses = assumption of one result by default,
hence no case for = =E2=80=98i =3D=3D 1=E2=80=99 in the code.

Added.


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

Resolves tarantool/tarantool#6371
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-6371-string= -char-no-arg
Issue: https://github.com/tarantool/tarantool/issues/6371
Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/gh-6371-str= ing-char-no-arg
Side note: CI is totally red, but = AFAICS it's unrelated with my patch.
Side note: See also = Changelog at the Tarantool branch.

src/lj_ffrecord.c =             &n= bsp;           &nbs= p;   |  2 ++
.../gh-6371-string-char-no-arg.test.lua =       | 28 +++++++++++++++++++
2 files changed, 30 insertions(+)
create mode = 100644 test/tarantool-tests/gh-6371-string-char-no-arg.test.lua

diff --git a/src/lj_ffrecord.c = b/src/lj_ffrecord.c
index 8dfa80ed..be890a93 100644
--- a/src/lj_ffrecord.c
+++ = b/src/lj_ffrecord.c
@@ -866,6 +866,8 @@ static void = LJ_FASTCALL recff_string_char(jit_State *J, RecordFFData *rd)
   for (i =3D 0; J->base[i] !=3D 0; i++)
     tr =3D emitir(IRT(IR_BUFPUT, = IRT_PGC), tr, J->base[i]);
   J->base[0] =3D emitir(IRT(IR_BUFSTR, = IRT_STR), tr, hdr);
+  } else if (i =3D=3D 0) {
+    J->base[0] =3D lj_ir_kstr(J, = &J2G(J)->strempty);
 }
 UNUSED(rd);
}
diff --git = a/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua = b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
new file mode 100644
index = 00000000..6df93f07
--- /dev/null
+++ = b/test/tarantool-tests/gh-6371-string-char-no-arg.test.lua
@@ -0,0 +1,28 @@
+local tap =3D = require('tap')
+
+-- Test file to = demonstrate assertion after `string.char()`
+-- = recording.
+-- See also, https://github.com/tarantool/tarantool/issues/6371.
+
+local test =3D = tap.test('gh-6371-string-char-no-arg')
+-- XXX: Number of = loop iterations.
+-- 1, 2 -- instruction becomes hot
+-- 3 -- trace is recorded (considering loop recording = specifics),
+-- but bytecodes are still executed via VM
+-- 4 -- trace is executed, need to check that emitted mcode = is
+--      correct
+local NTEST =3D 4
+test:plan(NTEST)
+
+-- Storage for the results to avoid trace = aborting by `test:ok()`.
+local results =3D {}
+jit.opt.start('hotloop=3D1')
+for _ =3D 1, = NTEST do
+  table.insert(results, string.char())
+end
+
+for i =3D 1, NTEST do
+  test:ok(results[i] =3D=3D '', 'correct recording of = string.char() without args')
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.31.0



-- 
Best = regards,
Sergey = Kaplun

= --Apple-Mail=_044512A5-617A-447A-A792-27B573104B99--