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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5125 bytes --]


Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
> 
>>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
>Typo: s/to executed/to execute
>Typo: s/wiht/with
>Typo: s/if emitted/if the emmited
>>+-- is incorrect, assertion guard type check will failed even for
>Typo: s/failed/fail
>Typo: s/for/ for the
>>+-- 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)
>Also, that test passes even without the patch on Linux x86_64 GC64: OFF
>>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,
>Maxim Kokryashkin

[-- Attachment #2: Type: text/html, Size: 7312 bytes --]

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

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!

Thanks for the review!

Fixed you comments.

On 06.12.22, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> > 

<snipped>

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

<snipped>

> >>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
> >Typo: s/to executed/to execute
> >Typo: s/wiht/with
> >Typo: s/if emitted/if the emmited

Fixed.

> >>+-- is incorrect, assertion guard type check will failed even for
> >Typo: s/failed/fail
> >Typo: s/for/ for the

Fixed.

See the iterative patch below:

===================================================================
diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
index 6ffc61fb..33794943 100644
--- a/test/tarantool-tests/lj-350-sload-typecheck.test.lua
+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
@@ -21,9 +21,9 @@ 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.
+-- Try to execute the compiled trace with IR_SLOAD, if the emitted
+-- mcode is incorrect, assertion guard type check will fail even
+-- for the correct type of argument and a new trace is recorded.
 jit.opt.start('hotloop=1', 'hotexit=1')
 
 jit.on()
===================================================================

Branch is force-pushed.

> >>+-- 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)
> >Also, that test passes even without the patch on Linux x86_64 GC64: OFF

Yes, because patch fixes the behaviour only for GC64 as mentioned in the
commit message.

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

<snipped>

> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun

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

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 4503 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 6 декабря 2022, 11:52 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Maxim!
>
>Thanks for the review!
>
>Fixed you comments.
>
>On 06.12.22, Maxim Kokryashkin wrote:
>>
>> Hi, Sergey!
>> Thanks for the patch!
>> Please consider my comments below.
>> > 
>
><snipped>
>
>> >>---
>> >>
>> >>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
>
><snipped>
>
>> >>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
>> >Typo: s/to executed/to execute
>> >Typo: s/wiht/with
>> >Typo: s/if emitted/if the emmited
>
>Fixed.
>
>> >>+-- is incorrect, assertion guard type check will failed even for
>> >Typo: s/failed/fail
>> >Typo: s/for/ for the
>
>Fixed.
>
>See the iterative patch below:
>
>===================================================================
>diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
>index 6ffc61fb..33794943 100644
>--- a/test/tarantool-tests/lj-350-sload-typecheck.test.lua
>+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
>@@ -21,9 +21,9 @@ 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.
>+-- Try to execute the compiled trace with IR_SLOAD, if the emitted
>+-- mcode is incorrect, assertion guard type check will fail even
>+-- for the correct type of argument and a new trace is recorded.
> jit.opt.start('hotloop=1', 'hotexit=1')
> 
> jit.on()
>===================================================================
>
>Branch is force-pushed.
>
>> >>+-- 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)
>> >Also, that test passes even without the patch on Linux x86_64 GC64: OFF
>
>Yes, because patch fixes the behaviour only for GC64 as mentioned in the
>commit message.
>
>> >>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
>
><snipped>
>
>> >>2.34.1
>> >--
>> >Best regards,
>> >Maxim Kokryashkin
>
>--
>Best regards,
>Sergey Kaplun
 

[-- Attachment #2: Type: text/html, Size: 6404 bytes --]

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

[sergos via Tarantool-patches]

Hi!

Thanks for the patch!

LGTM with fixes after Max’s review. 

Although, as we discussed earlier I would prefer the test has
`skipcond(not ffi.abi('gc64’), 'test is GC64 only')`

regards,
Sergos

> On 6 Dec 2022, at 11:49, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Maxim!
> 
> Thanks for the review!
> 
> Fixed you comments.
> 
> On 06.12.22, Maxim Kokryashkin wrote:
>> 
>> Hi, Sergey!
>> Thanks for the patch!
>> Please consider my comments below.
>>>  
> 
> <snipped>
> 
>>>> ---
>>>> 
>>>> 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
> 
> <snipped>
> 
>>>> 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
>>> Typo: s/to executed/to execute
>>> Typo: s/wiht/with
>>> Typo: s/if emitted/if the emmited
> 
> Fixed.
> 
>>>> +-- is incorrect, assertion guard type check will failed even for
>>> Typo: s/failed/fail
>>> Typo: s/for/ for the
> 
> Fixed.
> 
> See the iterative patch below:
> 
> ===================================================================
> diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
> index 6ffc61fb..33794943 100644
> --- a/test/tarantool-tests/lj-350-sload-typecheck.test.lua
> +++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
> @@ -21,9 +21,9 @@ 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.
> +-- Try to execute the compiled trace with IR_SLOAD, if the emitted
> +-- mcode is incorrect, assertion guard type check will fail even
> +-- for the correct type of argument and a new trace is recorded.
> jit.opt.start('hotloop=1', 'hotexit=1')
> 
> jit.on()
> ===================================================================
> 
> Branch is force-pushed.
> 
>>>> +-- 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)
>>> Also, that test passes even without the patch on Linux x86_64 GC64: OFF
> 
> Yes, because patch fixes the behaviour only for GC64 as mentioned in the
> commit message.
> 
>>>> 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
> 
> <snipped>
> 
>>>> 2.34.1
>>> --
>>> Best regards,
>>> Maxim Kokryashkin
> 
> -- 
> Best regards,
> Sergey Kaplun

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!

Thanks for the review!

On 15.12.22, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> 
> LGTM with fixes after Max’s review. 
> 
> Although, as we discussed earlier I would prefer the test has
> `skipcond(not ffi.abi('gc64’), 'test is GC64 only')`

Discussed with Igor offline and agreed to test this code flow for GC64
and non-GC64 modes.
So, ignoring.

> 
> regards,
> Sergos
> 

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin via Tarantool-patches]

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