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

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

Thanks for the patch!

I want to assure myself: the link to the internal JIRA doesn’t come to commit message?

If so - LGTM with changes after Igor’s review.

Sergos 


> On 12 Jul 2021, at 15:06, Sergey Kaplun <skaplun@tarantool.org> 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
> argument to store is not a single character. If it is, then it can be
> 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
> 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.
> 
> This patch changes the flag value to -129 to avoid intersections with
> any `char` 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
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index c2cf5a95..ab53fb47 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c
> @@ -1115,7 +1115,7 @@ static void asm_bufput(ASMState *as, IRIns *ir)
>   const CCallInfo *ci = &lj_ir_callinfo[IRCALL_lj_buf_putstr];
>   IRRef args[3];
>   IRIns *irs;
> -  int kchar = -1;
> +  int kchar = -129;
>   args[0] = ir->op1;  /* SBuf * */
>   args[1] = ir->op2;  /* GCstr * */
>   irs = IR(ir->op2);
> @@ -1123,7 +1123,7 @@ static void asm_bufput(ASMState *as, IRIns *ir)
>   if (irs->o == IR_KGC) {
>     GCstr *s = ir_kstr(irs);
>     if (s->len == 1) {  /* Optimize put of single-char string constant. */
> -      kchar = strdata(s)[0];
> +      kchar = (int8_t)strdata(s)[0];  /* Signed! */
>       args[1] = ASMREF_TMP1;  /* int, truncated to char */
>       ci = &lj_ir_callinfo[IRCALL_lj_buf_putchar];
>     }
> @@ -1150,7 +1150,7 @@ static void asm_bufput(ASMState *as, IRIns *ir)
>   asm_gencall(as, ci, args);
>   if (args[1] == ASMREF_TMP1) {
>     Reg tmp = ra_releasetmp(as, ASMREF_TMP1);
> -    if (kchar == -1)
> +    if (kchar == -129)
>       asm_tvptr(as, tmp, irs->op1);
>     else
>       ra_allockreg(as, kchar, tmp);
> 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)
> +
> +-- 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
> +  -- for -1. Fast function `string.char()` is recorded with
> +  -- 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')
> +end
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.31.0
>