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 98AF76F3D0; Wed, 1 Sep 2021 10:11:53 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 98AF76F3D0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1630480313; bh=n5Ys8IvrvgIfKVsYZn4GQm/3COj2IF3wRaEJeIOJM60=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Ep7o63lEE7ZtA7lll5J9tYozDXwNsp+Ovv68EPR73QrITCUxxJkLhAFssyG7At1T4 eGr4wADrWX4W+fQXA5yGirDPatv+kpKJF8qWpcl1eSm6vAeS2f4YR+nT3bFQ4tYee9 A+FptSPqwAWUyHoiIKN6ybd9RRP6NhxwGkcGnI4M= Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 7DEC36F3F6 for ; Wed, 1 Sep 2021 10:11:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7DEC36F3F6 Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1mLKP9-0006MY-Il; Wed, 01 Sep 2021 10:11:27 +0300 Date: Wed, 1 Sep 2021 10:10:06 +0300 To: Sergey Ostanevich Message-ID: References: <20210820154846.5515-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9EE8D69D3379D80C412F6C48019D6987C8600846831C518E9182A05F538085040C429199A27B6478E9F4BC998C40DEF1B79A5C619D99D4E17B44736AD55C87E7C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE705B093C0FC4B30B9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637129C704593A46970EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F225D1D71701B6A2F6BD829383ABC8770ECC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8062BEEFFB5F8EA3EF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA7E827F84554CEF5019E625A9149C048EE9ECD01F8117BC8BEE2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B53A69B3AC30C7B9475ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: 0D63561A33F958A547E72F7F606343C8E2DE2B3914ED06838108105EE628E164D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D340CB2836B823694490FA9B407A366F805FB5B4FFA7A4E078AD5D63D95DF296B4BF2DEEF642BAB8F6A1D7E09C32AA3244C26C938F43981AD4A5C7ED62127D0503A250262A5EE9971B0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojKvNUqyKacB0dSK5/8+/Erw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB42690775B68AE32EA7BFAE7F4A5FC489BFE6661525355B443F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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: =================================================================== 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’t 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 == 1` in the code. Sergey Kaplun: * added the description and the test for the problem Resolves tarantool/tarantool#6371 =================================================================== Branch is force-pushed. > > On 20 Aug 2021, at 18:48, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > (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’t handle this case. Fixed. > > > Each recording fast function expects 1 result by default. Hence, when > ^ > 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 ‘i == 1’ 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-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. > > > > 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 > > > > 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 = 0; J->base[i] != 0; i++) > > tr = emitir(IRT(IR_BUFPUT, IRT_PGC), tr, J->base[i]); > > J->base[0] = emitir(IRT(IR_BUFSTR, IRT_STR), tr, hdr); > > + } else if (i == 0) { > > + J->base[0] = 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 = require('tap') > > + > > +-- Test file to demonstrate assertion after `string.char()` > > +-- recording. > > +-- See also, https://github.com/tarantool/tarantool/issues/6371. > > + > > +local test = 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 = 4 > > +test:plan(NTEST) > > + > > +-- Storage for the results to avoid trace aborting by `test:ok()`. > > +local results = {} > > +jit.opt.start('hotloop=1') > > +for _ = 1, NTEST do > > + table.insert(results, string.char()) > > +end > > + > > +for i = 1, NTEST do > > + test:ok(results[i] == '', 'correct recording of string.char() without args') > > +end > > + > > +os.exit(test:check() and 0 or 1) > > -- > > 2.31.0 > > > -- Best regards, Sergey Kaplun