Re: [Tarantool-patches] [PATCH luajit] Fix IR_BUFPUT assembly.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@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