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 47DE76EC55; Tue, 20 Jul 2021 15:18:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 47DE76EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626783523; bh=R/8F/VStTbR09fXyUFPH/JD4x994g7DKLJtbJ8cNVhw=; 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=B2FtBthXtScdezJTg9zx3FosAFW0D7+uBGs6t1sqWF5F4S6ufpylANbNUOFCegxdJ ZYXYVVcW9yD2MUu6y5uQ2iAgTRRnTriJ2xlZy8rEBARv90lhcW2SxSFuz5Ebq/moVx 6+6fCodZ9QT9I3bG9JmdQ6Btahdu8aObBnFEaCvU= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 4E5EF6EC55 for ; Tue, 20 Jul 2021 15:18:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4E5EF6EC55 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1m5ohs-0006Ui-CC; Tue, 20 Jul 2021 15:18:40 +0300 Date: Tue, 20 Jul 2021 15:17:30 +0300 To: Igor Munkin Message-ID: References: <20210712120652.23695-1-skaplun@tarantool.org> <20210719222533.GG11494@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210719222533.GG11494@tarantool.org> X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3A9514C5AE4B3B389A94BDFA06D40730D182A05F538085040DC6946B0CAA8B2F78576BCCF61822AA25C64061044F8FEA6F7C7063AC10614E6 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3295C83650092F9EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063748E7A03516F25E8E8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8AE83A6EAF5C725742754099597E8D111117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B60A62CEF541B197C8089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A59ADE4C851AEE6308C412735A9C96DDA2722AC4664A23D5A1D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3458239C6F30CE7ACF5E68885305EA06C722CC956512281DBABCFE6F3B5763BC1A347C35A9856985211D7E09C32AA3244CF83E81D534B29C4E497EEC9F7B55C5F095A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojL49Xu4qyFBmkPXAPYAvIZA== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB43489F7EF796D421A87D86F4B9C3E592A3A8C72BA2FF686E1F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Fix IR_BUFPUT assembly. 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, Igor! Thanks for the review! I've updated branches with force-push. Side note: CI is green except start failure on arm64 [1]. But patch is unrelated to arm64 or GC64, so it seems OK. On 20.07.21, Igor Munkin wrote: > Sergey, > > Thanks for the patch! LGTM, except the several nits below. > > On 12.07.21, Sergey Kaplun wrote: > > From: Mike Pall > > > > Thanks to Peter Cawley. > > > > (cherry picked from commit 58d0dde0a2df49abc991decbabff15230010829a) > > > > When recording IR_BUFPTR special variable holds -1 value to mark that > > Typo: s/special variable/the special variable/. > > > argument to store is not a single character. If it is, then it can be > > Typo: s/to store/to be stored/. > > > stored in a register directly. When storing a single character we store > > it in the aforementioned variable first to reset the -1 value. But when > > I count 6 entries of 'store' in a different forms, so I propose to > reword the previous two sentences the following way: > | Otherwise, it can be stored directly in a register and this character > | is used to reset the hint via the aforementioned variable at first. > > > the system has signed characters, and the character to store equals > > \255, the check that the variable still holds -1 value becomes false > > positive and either wrong value is stored or the LuaJIT crashes. > > Also, I propose to reword the sentence above the following way: > | For the systems with signed `char` values, the case with the character > | being equal to \255 produces a false positive check and leads to > | either invalid value storing or even LuaJIT crash, since the variable > | with hint still holds -1 value. > > > > > This patch changes the flag value to -129 to avoid intersections with > > Minor: I believe 'collisions' are better than 'intersections' here. > > > any `char` values. > > Minor: s/`char`/one byte/. This is the main idea of the hack, AFAIU. > > > > > Sergey Kaplun: > > * added the description and the test for the problem The new commit message is the following: | Fix IR_BUFPUT assembly. | | Thanks to Peter Cawley. | | (cherry picked from commit 58d0dde0a2df49abc991decbabff15230010829a) | | When recording IR_BUFPTR the special variable holds -1 value to mark | that argument to be stored is not a single character. Otherwise, it can | be stored directly in a register, and this character is used to reset | the hint via the aforementioned variable at first. For the systems with | signed `char` values, the case with the character being equal to \255 | produces a false positive check and leads to either invalid value | storing or even LuaJIT crash, since the variable with hint still holds | -1 value. | | This patch changes the flag value to -129 to avoid collisions with | any one byte values. | | Sergey Kaplun: | * added the description and the test for the problem > > --- > > > > The patch fixes the problem described in TNT-142. > > > > Tarantool branch: https://github.com/tarantool/tarantool/tree/skaplun/lj-375-fix-ir-bufput > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-375-fix-ir-bufput > > Issue: https://github.com/LuaJIT/LuaJIT/issues/375 > > > > src/lj_asm.c | 6 +++--- > > .../lj-375-ir-bufput-signed-char.test.lua | 17 +++++++++++++++++ > > 2 files changed, 20 insertions(+), 3 deletions(-) > > create mode 100644 test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua > > > > > > > diff --git a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua > > new file mode 100644 > > index 00000000..8ac138f7 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua > > @@ -0,0 +1,17 @@ > > +local tap = require('tap') > > + > > +local test = tap.test('lj-375-ir-bufput-signed-char') > > +test:plan(3) > > This is a magic number, depending on the number of the loop iterations > below. Please consider introducing a variable for this constant, to make > further maintenance easier. Fixed with the following patch: =================================================================== diff --git a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua index 2ed6bfaf..2f5c3154 100644 --- a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua +++ b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua @@ -1,13 +1,14 @@ local tap = require('tap') local test = tap.test('lj-375-ir-bufput-signed-char') -test:plan(3) +local NTEST = 3 +test:plan(NTEST) -- Storage for the results to avoid trace aborting by `test:ok()`. local results = {} -- Avoid store forwarding optimization to store exactly 1 char. jit.opt.start(3, '-fwd', 'hotloop=1') -for _ = 1, 3 do +for _ = 1, NTEST do -- Check optimization for single char storing works correct -- for -1. Fast function `string.char()` is recorded with -- IR_BUFHDR and IR_BUFPUT IRs in case, when there are more than @@ -16,7 +17,7 @@ for _ = 1, 3 do table.insert(results, s:byte(1)) end -for i = 1, 3 do +for i = 1, NTEST do test:ok(results[i] == 0xff, 'correct -1 signed char assembling') end =================================================================== > > > + > > +-- Avoid store forwarding optimization to store exactly 1 char. > > +jit.opt.start(3, '-fwd', 'hotloop=1') > > +for _ = 1, 3 do > > + -- Check optimization for single char storing works correct > > Typo: s/for single char storing/for storing a single char/. > > > + -- for -1. Fast function `string.char()` is recorded with > > Minor: It's better use 0xff instead of -1 in this context. > > > + -- IR_BUFHDR and IR_BUFPUT IRs in case, when there are more than > > + -- 1 arguments. > > Typo: s/arguments/argument/. Applied all suggestions via the following patch: =================================================================== diff --git a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua index 2f5c3154..b7df88fd 100644 --- a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua +++ b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua @@ -9,16 +9,16 @@ local results = {} -- Avoid store forwarding optimization to store exactly 1 char. jit.opt.start(3, '-fwd', 'hotloop=1') for _ = 1, NTEST do - -- Check optimization for single char storing works correct - -- for -1. Fast function `string.char()` is recorded with + -- Check optimization for storing a single char works correct + -- for 0xff. Fast function `string.char()` is recorded with -- IR_BUFHDR and IR_BUFPUT IRs in case, when there are more than - -- 1 arguments. + -- 1 argument. local s = string.char(0xff, 0) table.insert(results, s:byte(1)) end for i = 1, NTEST do - test:ok(results[i] == 0xff, 'correct -1 signed char assembling') + test:ok(results[i] == 0xff, 'correct 0xff signed char assembling') end =================================================================== > > > + local s = string.char(0xff, 0) > > + test:ok(s:byte(1) == 0xff, 'correct -1 signed char assembling') > > Minor: I am concerned that test:ok might break the trace recording. > Could you please provide the trace dumps? > > To be sure the trace is recorded, you can flush everything before > running the loop and add the following assertion: > | test:ok(jit.util.traceinfo(1), ...) Indeed, fixed it with the following diff: =================================================================== diff --git a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua index 8ac138f7..2ed6bfaf 100644 --- a/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua +++ b/test/tarantool-tests/lj-375-ir-bufput-signed-char.test.lua @@ -3,6 +3,8 @@ local tap = require('tap') local test = tap.test('lj-375-ir-bufput-signed-char') test:plan(3) +-- Storage for the results to avoid trace aborting by `test:ok()`. +local results = {} -- Avoid store forwarding optimization to store exactly 1 char. jit.opt.start(3, '-fwd', 'hotloop=1') for _ = 1, 3 do @@ -11,7 +13,11 @@ for _ = 1, 3 do -- IR_BUFHDR and IR_BUFPUT IRs in case, when there are more than -- 1 arguments. local s = string.char(0xff, 0) - test:ok(s:byte(1) == 0xff, 'correct -1 signed char assembling') + table.insert(results, s:byte(1)) +end + +for i = 1, 3 do + test:ok(results[i] == 0xff, 'correct -1 signed char assembling') end os.exit(test:check() and 0 or 1) =================================================================== The dump is the following: | ---- TRACE 1 start lj-375-ir-bufput-signed-char.test.lua:11 | 0030 GGET 7 13 ; "string" | 0031 TGETS 7 7 14 ; "char" | 0032 KSHORT 8 255 | 0033 KSHORT 9 0 | 0034 CALL 7 2 3 | 0000 . FUNCC ; string.char | 0035 GGET 8 15 ; "table" | 0036 TGETS 8 8 16 ; "insert" | 0037 MOV 9 2 | 0038 MOV 11 7 | 0039 TGETS 10 7 17 ; "byte" | 0040 KSHORT 12 1 | 0041 CALL 10 0 3 | 0000 . FUNCC ; string.byte | 0042 CALLM 8 1 1 | 0000 . FUNCC ; table.insert | 0043 FORL 3 => 0030 > > > +end > > + > > +os.exit(test:check() and 0 or 1) > > -- > > 2.31.0 > > > > -- > Best regards, > IM [1]: https://github.com/tarantool/tarantool/runs/3113654187 -- Best regards, Sergey Kaplun