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 B3A876EC55; Wed, 21 Jul 2021 12:31:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B3A876EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626859876; bh=lAZZGonN6YFfi24jD0m6gJrq098spa/4HqWanSlWMLM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=ELKnIe+7x5elK1ac4Ma3xqGTBs+BCx7uADi5QFE7NGfEvmv7V2yJetT+YpK2A6nmm JCiq2iii9auYz0Eal00qe/Wwvy36hNHsbyrPdu23ybX3Tjca/Oc8iINZQOj/GHfXmk /Q5JqssR6sslKJa9aCzn3B+9zooyoO1x2cmxxfAg= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (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 E60A26EC55 for ; Wed, 21 Jul 2021 12:31:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E60A26EC55 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1m68ZN-0006Gd-VZ; Wed, 21 Jul 2021 12:31:14 +0300 Date: Wed, 21 Jul 2021 12:30:03 +0300 To: Igor Munkin , tarantool-patches@dev.tarantool.org 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: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C3A9514C5AE4B3B389A94BDFA06D40730D182A05F538085040F57FA08E0879BB26CCCCBB64A598D3D73AEDE0BAF971C23AA06809963120B41A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AED985C8E545F588EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CB87E27B59CE093D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8D56DBD4F45A705CD079B7A5257DF5F49117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A555425383A673B210A6F31359FB75449E5BAE84E95708CF8ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3435BBF0AC4E3A921C3F8E1CFDEEF020D8104E764321BCD76B9C55FDFBDC1DA58FA8D299E2E74C909B1D7E09C32AA3244C6BA41B7FB7134B7EF93193DF68ABC72363871F383B54D9B3729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJX8TSRcb/ShOM+WtYX9HSg== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB41BB5660CC05A2D8795A63398AFCC68D8ECD15413B45F0481F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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 Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" My bad, one more fix: Test is kinda flaky on x86 without GC64. Thanks Igor noticed that! It may no crash in some cases. Some clarification and fix below: IR has the following structure: |! 16 16 8 8 8 8 |! +-------+-------+---+---+---+---+ |! | op1 | op2 | t | o | r | s | |! +-------+-------+---+---+---+---+ |! | op12/i/gco32 | ot | prev | |! +-------+-------+---+---+---+---+ When the wrong branch is taken, the part of the random address from GCStr "\255" is taken as the op1 argument (ref for the other IR). It may happen, that IR(ref) from this arg is situated in the accessible memory, and test is not crashed. 3 iterations of the test is not enough to test JIT behaviour: 1 - instruction becomes hot 2,3 - trace is recorded (with the unrolling of the 2nd iteration), but VM executes regular bytecodes So we need 4th iteration executing incorrect emitting trace. See the following patch below: =================================================================== 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 b7df88fd..4d59ab04 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,7 +1,13 @@ local tap = require('tap') local test = tap.test('lj-375-ir-bufput-signed-char') -local NTEST = 3 +-- XXX: Number of loop iterations. +-- 1 -- instruction becomes hot +-- 2, 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()`. =================================================================== Branch is force-pushed. Check on old version with the following command: | i=0; while [[ $? != 0 ]]; do i=$[$i+1]; if [ $i -gt 1000 ]; then break; fi; echo $i; make tarantool-tests>/dev/null; done | 1 | ... | 1000 On 20.07.21, Sergey Kaplun via Tarantool-patches wrote: > 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 -- Best regards, Sergey Kaplun