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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.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