Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
@ 2022-12-02  8:42 Sergey Kaplun via Tarantool-patches
  2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
  2023-01-12 14:55 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-12-02  8:42 UTC (permalink / raw)
  To: Sergey Ostanevich, Maxim Kokryashkin; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-02  8:42 [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD Sergey Kaplun via Tarantool-patches
@ 2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
  2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
  2023-01-12 14:55 ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-12-06  7:08 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
@ 2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
  2022-12-06 13:06     ` Maxim Kokryashkin via Tarantool-patches
  2022-12-15 15:49     ` sergos via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-12-06  8:49 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
@ 2022-12-06 13:06     ` Maxim Kokryashkin via Tarantool-patches
  2022-12-15 15:49     ` sergos via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-12-06 13:06 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
  2022-12-06 13:06     ` Maxim Kokryashkin via Tarantool-patches
@ 2022-12-15 15:49     ` sergos via Tarantool-patches
  2022-12-16  8:13       ` Sergey Kaplun via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: sergos via Tarantool-patches @ 2022-12-15 15:49 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-15 15:49     ` sergos via Tarantool-patches
@ 2022-12-16  8:13       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-12-16  8:13 UTC (permalink / raw)
  To: sergos; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD.
  2022-12-02  8:42 [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD Sergey Kaplun via Tarantool-patches
  2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-01-12 14:55 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-01-12 14:55 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-01-12 15:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02  8:42 [Tarantool-patches] [PATCH luajit] x64/LJ_GC64: Fix type-check-only variant of SLOAD Sergey Kaplun via Tarantool-patches
2022-12-06  7:08 ` Maxim Kokryashkin via Tarantool-patches
2022-12-06  8:49   ` Sergey Kaplun via Tarantool-patches
2022-12-06 13:06     ` Maxim Kokryashkin via Tarantool-patches
2022-12-15 15:49     ` sergos via Tarantool-patches
2022-12-16  8:13       ` Sergey Kaplun via Tarantool-patches
2023-01-12 14:55 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox