From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
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 <tarantool-patches@dev.tarantool.org>;
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
<skaplun@tarantool.org>)
id 1m5ohs-0006Ui-CC; Tue, 20 Jul 2021 15:18:40 +0300
Date: Tue, 20 Jul 2021 15:17:30 +0300
To: Igor Munkin <imun@tarantool.org>
Message-ID: <YPa+2pilAGHb8CtK@root>
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 <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
Reply-To: Sergey Kaplun <skaplun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@dev.tarantool.org>
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 <mike>
> >
> > 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
> >
>
> <snipped>
>
> > 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