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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

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