[Tarantool-patches] [PATCH luajit] Fix IR_BUFPUT assembly.
Sergey Kaplun
skaplun at tarantool.org
Tue Jul 20 15:17:30 MSK 2021
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
More information about the Tarantool-patches
mailing list