<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>Please consider my comments below.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16699707341032742397_BODY">From: Mike Pall <mike><br><br>Thanks to Peter Cawley.<br><br>(cherry picked from commit 05fbdf565c700365d22e38f11478101a0d92a23e)<br><br>To reproduce the issue we need to assemble `IR_SLOAD` with<br>`IRSLOAD_TYPECHECK` flag set. Also, this IR shouldn't be used later and<br>be not pri, not any number, not any integer type. For GC64 mode, we get<br>the following mcode for this typecheck only variant of the IR:<br><br>| 55557f6bffc9 mov rdi, [rdx+0x4]<br>| 55557f6bffcd sar rdi, 0x2f<br>| 55557f6bffd1 cmp edi, -0x0b<br>| 55557f6bffd4 jnz 0x55557f6b0010 ->0<br><br>This 0x4 addition is excess and crucial:<br>We got the invalid irtype value to compare (due wrong addressing) -- so<br>the assertion guard is always failed and we always exit from the trace.<br><br>This patch removes this addition.<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#7230<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>@@ -1759,7 +1759,7 @@ static void asm_sload(ASMState *as, IRIns *ir)<br> emit_i8(as, irt_toitype(t));<br> emit_rr(as, XO_ARITHi8, XOg_CMP, tmp);<br> emit_shifti(as, XOg_SAR|REX_64, tmp, 47);<br>- emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs+4);<br>+ emit_rmro(as, XO_MOV, tmp|REX_64, base, ofs);<br> #else<br> } else {<br> emit_i8(as, irt_toitype(t));<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</div></div></div></div></blockquote><div>Typo: s/to executed/to execute</div><div>Typo: s/wiht/with</div><div>Typo: s/if emitted/if the emmited</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- is incorrect, assertion guard type check will failed even for</div></div></div></div></blockquote><div>Typo: s/failed/fail</div><div>Typo: s/for/ for the</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- 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)</div></div></div></div></blockquote><div>Also, that test passes even without the patch on Linux x86_64 GC64: OFF</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>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>@@ -6,7 +6,6 @@ local tap = require('tap')<br> -- conversions.<br> -- See also <a href="https://github.com/LuaJIT/LuaJIT/issues/408" target="_blank">https://github.com/LuaJIT/LuaJIT/issues/408</a>,<br> -- <a href="https://github.com/LuaJIT/LuaJIT/pull/412" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/412</a>,<br>--- <a href="https://github.com/LuaJIT/LuaJIT/pull/412" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/412</a>,<br> -- <a href="https://github.com/tarantool/tarantool/issues/7655" target="_blank">https://github.com/tarantool/tarantool/issues/7655</a>.<br> local test = tap.test('lj-408-tonumber-cdata-record')<br> <br>@@ -14,15 +13,6 @@ local NULL = ffi.cast('void *', 0)<br> <br> test:plan(4)<br> <br>--- This test won't fail for GC64 on x86_64. This happens due to<br>--- wrong instruction emitting for SLOAD IR -- we always exit by<br>--- the assertion guard on the argument type check. See also<br>--- <a href="https://github.com/LuaJIT/LuaJIT/pull/350" target="_blank">https://github.com/LuaJIT/LuaJIT/pull/350</a>.<br>--- The test fails without fix in the current commit, if the<br>--- following commit is backported:<br>--- <a href="https://github.com/LuaJIT/LuaJIT/commit/05fbdf56" target="_blank">https://github.com/LuaJIT/LuaJIT/commit/05fbdf56</a><br>--- Feel free to remove this comment after backporting of the<br>--- aforementioned commit.<br> local function check(x)<br> -- Don't use a tail call to avoid "leaving loop in root trace"<br> -- error, so the trace will be compiled.<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>