

<HTML><BODY><div>Hi!</div><div>Thanks for the fixes!</div><div>LGTM</div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div><div> </div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 6 декабря 2022, 11:52 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16703167431880185358_BODY">Hi, Maxim!<br><br>Thanks for the review!<br><br>Fixed you comments.<br><br>On 06.12.22, Maxim Kokryashkin wrote:<br>><br>> Hi, Sergey!<br>> Thanks for the patch!<br>> Please consider my comments below.<br>> > <br><br><snipped><br><br>> >>---<br>> >><br>> >>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/lj-350-fix-sload-typecheck-full-ci" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/lj-350-fix-sload-typecheck-full-ci</a><br>> >>Issues:<br>> >>* <a href="https://github.com/tarantool/tarantool/issues/7230" target="_blank">https://github.com/tarantool/tarantool/issues/7230</a><br>> >>* <a href="https://github.com/LuaJIT/LuaJIT/pull/350" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/350</a><br>> >>Tarantool PR: <a href="https://github.com/tarantool/tarantool/pull/7995" target="_blank">https://github.com/tarantool/tarantool/pull/7995</a><br>> >><br>> >> src/lj_asm_x86.h | 2 +-<br>> >> .../lj-350-sload-typecheck.test.lua | 42 +++++++++++++++++++<br>> >> .../lj-408-tonumber-cdata-record.test.lua | 10 -----<br>> >> 3 files changed, 43 insertions(+), 11 deletions(-)<br>> >> create mode 100644 test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>> >><br>> >>diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h<br>> >>index 8a4d4025..8efda8e5 100644<br>> >>--- a/src/lj_asm_x86.h<br>> >>+++ b/src/lj_asm_x86.h<br><br><snipped><br><br>> >>diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>> >>new file mode 100644<br>> >>index 00000000..6ffc61fb<br>> >>--- /dev/null<br>> >>+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>> >>@@ -0,0 +1,42 @@<br>> >>+local tap = require('tap')<br>> >>+local traceinfo = require('jit.util').traceinfo<br>> >>+<br>> >>+-- Test file to demonstrate the incorrect GC64 JIT asembling<br>> >>+-- `IR_SLOAD`.<br>> >>+-- See also <a href="https://github.com/LuaJIT/LuaJIT/pull/350" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/350</a> .<br>> >>+local test = tap.test('lj-350-sload-typecheck')<br>> >>+<br>> >>+test:plan(1)<br>> >>+<br>> >>+-- Contains only IR_SLOAD after recording.<br>> >>+local function sload(arg)<br>> >>+ return arg<br>> >>+end<br>> >>+<br>> >>+local tab_arg = {}<br>> >>+<br>> >>+-- Reset JIT, remove any other traces.<br>> >>+jit.off()<br>> >>+jit.flush()<br>> >>+<br>> >>+assert(not traceinfo(1), 'no traces compiled after flush')<br>> >>+<br>> >>+-- Try to executed compiled trace wiht IR_SLOAD, if emitted mcode<br>> >Typo: s/to executed/to execute<br>> >Typo: s/wiht/with<br>> >Typo: s/if emitted/if the emmited<br><br>Fixed.<br><br>> >>+-- is incorrect, assertion guard type check will failed even for<br>> >Typo: s/failed/fail<br>> >Typo: s/for/ for the<br><br>Fixed.<br><br>See the iterative patch below:<br><br>===================================================================<br>diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua b/test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>index 6ffc61fb..33794943 100644<br>--- a/test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua<br>@@ -21,9 +21,9 @@ jit.flush()<br> <br> assert(not traceinfo(1), 'no traces compiled after flush')<br> <br>--- Try to executed compiled trace wiht IR_SLOAD, if emitted mcode<br>--- is incorrect, assertion guard type check will failed even for<br>--- correct type of argument and a new trace is recorded.<br>+-- Try to execute the compiled trace with IR_SLOAD, if the emitted<br>+-- mcode is incorrect, assertion guard type check will fail even<br>+-- for the correct type of argument and a new trace is recorded.<br> jit.opt.start('hotloop=1', 'hotexit=1')<br> <br> jit.on()<br>===================================================================<br><br>Branch is force-pushed.<br><br>> >>+-- correct type of argument and a new trace is recorded.<br>> >>+jit.opt.start('hotloop=1', 'hotexit=1')<br>> >>+<br>> >>+jit.on()<br>> >>+<br>> >>+-- Make the function hot.<br>> >>+sload(tab_arg)<br>> >>+-- Compile the trace.<br>> >>+sload(tab_arg)<br>> >>+-- Execute trace and try to compile a trace from the side exit.<br>> >>+sload(tab_arg)<br>> >>+<br>> >>+jit.off()<br>> >>+<br>> >>+test:ok(not traceinfo(2), 'the second trace should not be compiled')<br>> >>+<br>> >>+os.exit(test:check() and 0 or 1)<br>> >Also, that test passes even without the patch on Linux x86_64 GC64: OFF<br><br>Yes, because patch fixes the behaviour only for GC64 as mentioned in the<br>commit message.<br><br>> >>diff --git a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua<br>> >>index bf9e8e46..a8235e93 100644<br>> >>--- a/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua<br>> >>+++ b/test/tarantool-tests/lj-408-tonumber-cdata-record.test.lua<br><br><snipped><br><br>> >>2.34.1<br>> >--<br>> >Best regards,<br>> >Maxim Kokryashkin<br><br>--<br>Best regards,<br>Sergey Kaplun</div></div></div></div></blockquote><div> </div></BODY></HTML>