<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM, except for a few nits below and the single question.</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_16812456431987006540_BODY">From: Mike Pall <mike><br><br>Reported by Yichun Zhang.<br><br>(cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)<br><br>The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered<br>as 64 bit address. On GC64 mode it may lead to the following assembly:</div></div></div></div></blockquote><div>Typo: s/as 64 bit/a 64-bit/</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>| mov eax, edi<br>so, high 32 bits of the reference are lost.</div></div></div></div></blockquote><div>Typo: s/high/the high/</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><br>This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`<br>64 bit long. Now the resulting assembly is the following:<br>| mov rax, rdi<br><br>False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`<br>already initialized as 0.<br><br>False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,<br><src/lj_record.c> is OK, since `REF_NIL` is the last reference before<br>`REF_BASE` and this iteration of a cycle is still the last one.<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8516<br>---<br><br>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l</a><br>Related issues:<br>* <a href="https://github.com/openresty/lua-resty-core/issues/144" target="_blank">https://github.com/openresty/lua-resty-core/issues/144</a><br>* <a href="https://github.com/tarantool/tarantool/issues/8516" target="_blank">https://github.com/tarantool/tarantool/issues/8516</a><br>PR: <a href="https://github.com/tarantool/tarantool/pull/8553" target="_blank">https://github.com/tarantool/tarantool/pull/8553</a><br>ML: <a href="https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode" target="_blank">https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode</a><br><br>Don't restrict test case by GC64 mode, because want to test `IR_LREF`<br>for any mode. Keep GC64 in the test name, to be clear where expect the<br>SegFault.<br><br> src/lj_asm.c | 1 +<br> src/lj_ir.h | 4 ++-<br> src/lj_opt_sink.c | 1 +<br> .../or-144-gc64-asmref-l.test.lua | 28 +++++++++++++++++++<br> 4 files changed, 33 insertions(+), 1 deletion(-)<br> create mode 100644 test/tarantool-tests/or-144-gc64-asmref-l.test.lua<br><br>diff --git a/src/lj_asm.c b/src/lj_asm.c<br>index a154547b..fd31cd04 100644<br>--- a/src/lj_asm.c<br>+++ b/src/lj_asm.c<br>@@ -2013,6 +2013,7 @@ static void asm_setup_regsp(ASMState *as)<br> ir->prev = REGSP_INIT;<br> if (irt_is64(ir->t) && ir->o != IR_KNULL) {<br> #if LJ_GC64<br>+ /* The false-positive of irt_is64() for ASMREF_L (REF_NIL) is OK here. */<br> ir->i = 0; /* Will become non-zero only for RIP-relative addresses. */<br> #else<br> /* Make life easier for backends by putting address of constant in i. */<br>diff --git a/src/lj_ir.h b/src/lj_ir.h<br>index 4bad47ed..e8bca275 100644<br>--- a/src/lj_ir.h<br>+++ b/src/lj_ir.h<br>@@ -375,10 +375,12 @@ typedef struct IRType1 { uint8_t irt; } IRType1;<br> #define irt_isint64(t) (irt_typerange((t), IRT_I64, IRT_U64))<br> <br> #if LJ_GC64<br>+/* Include IRT_NIL, so IR(ASMREF_L) (aka REF_NIL) is considered 64 bit. */<br> #define IRT_IS64 \<br> ((1u<<IRT_NUM)|(1u<<IRT_I64)|(1u<<IRT_U64)|(1u<<IRT_P64)|\<br> (1u<<IRT_LIGHTUD)|(1u<<IRT_STR)|(1u<<IRT_THREAD)|(1u<<IRT_PROTO)|\<br>- (1u<<IRT_FUNC)|(1u<<IRT_CDATA)|(1u<<IRT_TAB)|(1u<<IRT_UDATA))<br>+ (1u<<IRT_FUNC)|(1u<<IRT_CDATA)|(1u<<IRT_TAB)|(1u<<IRT_UDATA)|\<br>+ (1u<<IRT_NIL))<br> #elif LJ_64<br> #define IRT_IS64 \<br> ((1u<<IRT_NUM)|(1u<<IRT_I64)|(1u<<IRT_U64)|(1u<<IRT_P64)|(1u<<IRT_LIGHTUD))<br>diff --git a/src/lj_opt_sink.c b/src/lj_opt_sink.c<br>index 929ccb61..a16d112f 100644<br>--- a/src/lj_opt_sink.c<br>+++ b/src/lj_opt_sink.c<br>@@ -219,6 +219,7 @@ static void sink_sweep_ins(jit_State *J)<br> for (ir = IR(J->cur.nk); ir < irbase; ir++) {<br> irt_clearmark(ir->t);<br> ir->prev = REGSP_INIT;<br>+ /* The false-positive of irt_is64() for ASMREF_L (REF_NIL) is OK here. */<br> if (irt_is64(ir->t) && ir->o != IR_KNULL)<br> ir++;<br> }<br>diff --git a/test/tarantool-tests/or-144-gc64-asmref-l.test.lua b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua<br>new file mode 100644<br>index 00000000..0c352c29<br>--- /dev/null<br>+++ b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua<br>@@ -0,0 +1,28 @@<br>+local tap = require('tap')<br>+local test = tap.test('or-144-gc64-asmref-l'):skipcond({<br>+ ['Test requires JIT enabled'] = not jit.status(),<br>+})<br>+<br>+test:plan(1)<br>+<br>+-- Test file to demonstrate LuaJIT `IR_LREF` assembling incorrect<br>+-- behaviour.<br>+-- See also:<br>+-- * <a href="https://github.com/openresty/lua-resty-core/issues/144" target="_blank">https://github.com/openresty/lua-resty-core/issues/144</a>.<br>+-- * <a href="https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode" target="_blank">https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode</a>.<br>+<br>+jit.opt.start('hotloop=1')<br>+<br>+local global_env<br>+local _<br>+for i = 1, 4 do<br>+ -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).<br>+ global_env = getfenv(0)<br>+ -- Need to reuse the register, to cause emitting of `mov`<br>+ -- instruction (see `ra_left()` in <src/lj_asm.c>).<br>+ _ = tostring(i)<br>+end<br>+<br>+test:ok(global_env == getfenv(0), 'IR_LREF assembling correctness')<br>+<br>+os.exit(test:check() and 0 or 1)</div></div></div></div></blockquote><div>Neither this test case, nor the original one from OpenResty fail before the patch on OSX/ARM64.</div><div>Is it expected behavior or not?</div><div>On x86 GC64 it behaves as expected though.</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>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div><div> </div></div></blockquote></BODY></HTML>