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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 <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

-- 
Best regards,
Sergey Kaplun