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 01EFA6EC55; Tue, 20 Jul 2021 01:49:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 01EFA6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626734950; bh=6G0NGx94+nOPF4TUzBLQYixPjbLz1HlxJhcV1T5X3Jc=; 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=ESOh/kWL6UlHDrgviBDqrgE3sZyxVG9X1kcdNe3CI5JAH6Io3GGk/uN9U5Gxqh8/+ K2nEkkXGB9ekApMdlkhCYYC6QOEjIKmXe+vTxdwD56B5CJ44dpKUqvZIJOVKrI5DWk Pb9xXhN64++OHBakty26BjBsBIgyY7gH2Pmkszjg= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 7EA726EC55 for ; Tue, 20 Jul 2021 01:49:08 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 7EA726EC55 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1m5c4R-0003i2-C8; Tue, 20 Jul 2021 01:49:07 +0300 Date: Tue, 20 Jul 2021 01:25:33 +0300 To: Sergey Kaplun Message-ID: <20210719222533.GG11494@tarantool.org> References: <20210712120652.23695-1-skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210712120652.23695-1-skaplun@tarantool.org> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C320FB367CC9CBEE800EEBD3117ABA9A5B182A05F53808504032AE76ACDE2EC6DFBE242CEFD6E9D0322811D3CBB4E2625E27D6236CB32F44D5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CE4525FFB91B9BBCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063706922F90966A37BA8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82A77FBF5B8504704F825D7AA89EE9E40117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC3A703B70628EAD7BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751FCB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D635BA3ABDB36C18089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A580C7E41B46EAA1F953267B86261F3E32A4CB26F2A4B00E0ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3482189E76C6218D8FDE3B1A89A613BD46855347A2FDF8455F4368519A79B39899C88ED9D13B83E5911D7E09C32AA3244C16CC349092F8F3228C5136A7C74532CB3C6EB905E3A8056B927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojL49Xu4qyFBlomGeU90OgUw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D110A43F91800FE2AF8CD62CE664CD94FA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 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. > + > +-- 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/. > + 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), ...) > +end > + > +os.exit(test:check() and 0 or 1) > -- > 2.31.0 > -- Best regards, IM