Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.

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

Sergey,

I've checked the patches into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 02.12.22, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Thanks to Peter Cawley.
> 
> (cherry picked from commit 05fbdf565c700365d22e38f11478101a0d92a23e)
> 
> To reproduce the issue we need to assemble `IR_SLOAD` with
> `IRSLOAD_TYPECHECK` flag set. Also, this IR shouldn't be used later and
> be not pri, not any number, not any integer type. For GC64 mode, we get
> the following mcode for this typecheck only variant of the IR:
> 
> | 55557f6bffc9  mov rdi, [rdx+0x4]
> | 55557f6bffcd  sar rdi, 0x2f
> | 55557f6bffd1  cmp edi, -0x0b
> | 55557f6bffd4  jnz 0x55557f6b0010        ->0
> 
> This 0x4 addition is excess and crucial:
> We got the invalid irtype value to compare (due wrong addressing) -- so
> the assertion guard is always failed and we always exit from the trace.
> 
> This patch removes this addition.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#7230
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-350-fix-sload-typecheck-full-ci
> Issues:
> * https://github.com/tarantool/tarantool/issues/7230
> * https://github.com/LuaJIT/LuaJIT/pull/350
> Tarantool PR: https://github.com/tarantool/tarantool/pull/7995
> 
>  src/lj_asm_x86.h                              |  2 +-
>  .../lj-350-sload-typecheck.test.lua           | 42 +++++++++++++++++++
>  .../lj-408-tonumber-cdata-record.test.lua     | 10 -----
>  3 files changed, 43 insertions(+), 11 deletions(-)
>  create mode 100644 test/tarantool-tests/lj-350-sload-typecheck.test.lua
> 
> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 8a4d4025..8efda8e5 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h
> @@ -1759,7 +1759,7 @@ static void asm_sload(ASMState *as, IRIns *ir)
>        emit_i8(as, irt_toitype(t));
>        emit_rr(as, XO_ARITHi8, XOg_CMP, tmp);
>        emit_shifti(as, XOg_SAR|REX_64, tmp, 47);
> -      emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs+4);
> +      emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs);
>  #else
>      } else {
>        emit_i8(as, irt_toitype(t));
> diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
> new file mode 100644
> index 00000000..6ffc61fb
> --- /dev/null
> +++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
> @@ -0,0 +1,42 @@
> +local tap = require('tap')
> +local traceinfo = require('jit.util').traceinfo
> +
> +-- Test file to demonstrate the incorrect GC64 JIT asembling
> +-- `IR_SLOAD`.
> +-- See also https://github.com/LuaJIT/LuaJIT/pull/350.
> +local test = tap.test('lj-350-sload-typecheck')
> +
> +test:plan(1)
> +
> +-- Contains only IR_SLOAD after recording.
> +local function sload(arg)
> +  return arg
> +end
> +
> +local tab_arg = {}
> +
> +-- Reset JIT, remove any other traces.
> +jit.off()
> +jit.flush()
> +
> +assert(not traceinfo(1), 'no traces compiled after flush')
> +
> +-- Try to executed compiled trace wiht IR_SLOAD, if emitted mcode
> +-- is incorrect, assertion guard type check will failed even for
> +-- correct type of argument and a new trace is recorded.
> +jit.opt.start('hotloop=1', 'hotexit=1')
> +
> +jit.on()
> +
> +-- Make the function hot.
> +sload(tab_arg)
> +-- Compile the trace.
> +sload(tab_arg)
> +-- Execute trace and try to compile a trace from the side exit.
> +sload(tab_arg)
> +
> +jit.off()
> +
> +test:ok(not traceinfo(2), 'the second trace should not be compiled')
> +
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
> index bf9e8e46..a8235e93 100644
> --- a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
> +++ b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua
> @@ -6,7 +6,6 @@ local tap = require('tap')
>  -- conversions.
>  -- See also https://github.com/LuaJIT/LuaJIT/issues/408,
>  -- https://github.com/LuaJIT/LuaJIT/pull/412,
> --- https://github.com/LuaJIT/LuaJIT/pull/412,
>  -- https://github.com/tarantool/tarantool/issues/7655.
>  local test = tap.test('lj-408-tonumber-cdata-record')
>  
> @@ -14,15 +13,6 @@ local NULL = ffi.cast('void *', 0)
>  
>  test:plan(4)
>  
> --- This test won't fail for GC64 on x86_64. This happens due to
> --- wrong instruction emitting for SLOAD IR -- we always exit by
> --- the assertion guard on the argument type check. See also
> --- https://github.com/LuaJIT/LuaJIT/pull/350.
> --- The test fails without fix in the current commit, if the
> --- following commit is backported:
> --- https://github.com/LuaJIT/LuaJIT/commit/05fbdf56
> --- Feel free to remove this comment after backporting of the
> --- aforementioned commit.
>  local function check(x)
>    -- Don't use a tail call to avoid "leaving loop in root trace"
>    -- error, so the trace will be compiled.
> -- 
> 2.34.1
> 

-- 
Best regards,
IM