[Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Yichun Zhang.

(cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)

The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered
as 64 bit address. On GC64 mode it may lead to the following assembly:
| mov eax, edi
so, high 32 bits of the reference are lost.

This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`
64 bit long. Now the resulting assembly is the following:
| mov rax, rdi

False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`
already initialized as 0.

False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,
<src/lj_record.c> is OK, since `REF_NIL` is the last reference before
`REF_BASE` and this iteration of a cycle is still the last one.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#8516
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l
Related issues:
* https://github.com/openresty/lua-resty-core/issues/144
* https://github.com/tarantool/tarantool/issues/8516
PR: https://github.com/tarantool/tarantool/pull/8553
ML: https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode

Don't restrict test case by GC64 mode, because want to test `IR_LREF`
for any mode. Keep GC64 in the test name, to be clear where expect the
SegFault.

 src/lj_asm.c                                  |  1 +
 src/lj_ir.h                                   |  4 ++-
 src/lj_opt_sink.c                             |  1 +
 .../or-144-gc64-asmref-l.test.lua             | 28 +++++++++++++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/or-144-gc64-asmref-l.test.lua

diff --git a/src/lj_asm.c b/src/lj_asm.c
index a154547b..fd31cd04 100644
--- a/src/lj_asm.c
+++ b/src/lj_asm.c
@@ -2013,6 +2013,7 @@ static void asm_setup_regsp(ASMState *as)
     ir->prev = REGSP_INIT;
     if (irt_is64(ir->t) && ir->o != IR_KNULL) {
 #if LJ_GC64
+      /* The false-positive of irt_is64() for ASMREF_L (REF_NIL) is OK here. */
       ir->i = 0;  /* Will become non-zero only for RIP-relative addresses. */
 #else
       /* Make life easier for backends by putting address of constant in i. */
diff --git a/src/lj_ir.h b/src/lj_ir.h
index 4bad47ed..e8bca275 100644
--- a/src/lj_ir.h
+++ b/src/lj_ir.h
@@ -375,10 +375,12 @@ typedef struct IRType1 { uint8_t irt; } IRType1;
 #define irt_isint64(t)		(irt_typerange((t), IRT_I64, IRT_U64))
 
 #if LJ_GC64
+/* Include IRT_NIL, so IR(ASMREF_L) (aka REF_NIL) is considered 64 bit. */
 #define IRT_IS64 \
   ((1u<<IRT_NUM)|(1u<<IRT_I64)|(1u<<IRT_U64)|(1u<<IRT_P64)|\
    (1u<<IRT_LIGHTUD)|(1u<<IRT_STR)|(1u<<IRT_THREAD)|(1u<<IRT_PROTO)|\
-   (1u<<IRT_FUNC)|(1u<<IRT_CDATA)|(1u<<IRT_TAB)|(1u<<IRT_UDATA))
+   (1u<<IRT_FUNC)|(1u<<IRT_CDATA)|(1u<<IRT_TAB)|(1u<<IRT_UDATA)|\
+   (1u<<IRT_NIL))
 #elif LJ_64
 #define IRT_IS64 \
   ((1u<<IRT_NUM)|(1u<<IRT_I64)|(1u<<IRT_U64)|(1u<<IRT_P64)|(1u<<IRT_LIGHTUD))
diff --git a/src/lj_opt_sink.c b/src/lj_opt_sink.c
index 929ccb61..a16d112f 100644
--- a/src/lj_opt_sink.c
+++ b/src/lj_opt_sink.c
@@ -219,6 +219,7 @@ static void sink_sweep_ins(jit_State *J)
   for (ir = IR(J->cur.nk); ir < irbase; ir++) {
     irt_clearmark(ir->t);
     ir->prev = REGSP_INIT;
+    /* The false-positive of irt_is64() for ASMREF_L (REF_NIL) is OK here. */
     if (irt_is64(ir->t) && ir->o != IR_KNULL)
       ir++;
   }
diff --git a/test/tarantool-tests/or-144-gc64-asmref-l.test.lua b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua
new file mode 100644
index 00000000..0c352c29
--- /dev/null
+++ b/test/tarantool-tests/or-144-gc64-asmref-l.test.lua
@@ -0,0 +1,28 @@
+local tap = require('tap')
+local test = tap.test('or-144-gc64-asmref-l'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+-- Test file to demonstrate LuaJIT `IR_LREF` assembling incorrect
+-- behaviour.
+-- See also:
+-- * https://github.com/openresty/lua-resty-core/issues/144.
+-- * https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode.
+
+jit.opt.start('hotloop=1')
+
+local global_env
+local _
+for i = 1, 4 do
+  -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).
+  global_env = getfenv(0)
+  -- Need to reuse the register, to cause emitting of `mov`
+  -- instruction (see `ra_left()` in <src/lj_asm.c>).
+  _ = tostring(i)
+end
+
+test:ok(global_env == getfenv(0), 'IR_LREF assembling correctness')
+
+os.exit(test:check() and 0 or 1)
-- 
2.34.1

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 5121 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 7432 bytes --]

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Sergey Kaplun via Tarantool-patches]

Hi, Max!
Thanks for the review!

On 18.04.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below and the single question.
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Yichun Zhang.
> >>
> >>(cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)
> >>
> >>The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered
> >>as 64 bit address. On GC64 mode it may lead to the following assembly:
> >Typo: s/as 64 bit/a 64-bit/

Fixed, thanks!

> >>| mov eax, edi
> >>so, high 32 bits of the reference are lost.
> >Typo: s/high/the high/
> >>
> >>This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`
> >>64 bit long. Now the resulting assembly is the following:
> >>| mov rax, rdi

Fixed, thanks!

Branch is force-pushed.

> >>
> >>False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`
> >>already initialized as 0.
> >>
> >>False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,
> >><src/lj_record.c> is OK, since `REF_NIL` is the last reference before
> >>`REF_BASE` and this iteration of a cycle is still the last one.
> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>---
> >>
> >>Branch:  https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l
> >>Related issues:
> >>*  https://github.com/openresty/lua-resty-core/issues/144
> >>*  https://github.com/tarantool/tarantool/issues/8516
> >>PR:  https://github.com/tarantool/tarantool/pull/8553
> >>ML:  https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode
> >>

<snipped>

> >>+local global_env
> >>+local _
> >>+for i = 1, 4 do
> >>+ -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).
> >>+ global_env = getfenv(0)
> >>+ -- Need to reuse the register, to cause emitting of `mov`
> >>+ -- instruction (see `ra_left()` in <src/lj_asm.c>).
> >>+ _ = tostring(i)
> >>+end
> >>+
> >>+test:ok(global_env == getfenv(0), 'IR_LREF assembling correctness')
> >>+
> >>+os.exit(test:check() and 0 or 1)
> >Neither this test case, nor the original one from OpenResty fail before the patch on OSX/ARM64.
> >Is it expected behavior or not?

Yes, I think that non x86_64 arches are unaffected, since they use
`ra_leftov()` instead.

> >On x86 GC64 it behaves as expected though.
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> > 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[sergos via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2899 bytes --]

Hi!

Thanks for the patch!

Since test is x86_64 only - can we put an explicit skipcond then?

Otherwise LGTM.

Sergos.

> On 2 May 2023, at 11:13, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Hi, Max!
> Thanks for the review!
> 
> On 18.04.23, Maxim Kokryashkin wrote:
>> 
>> Hi, Sergey!
>> Thanks for the patch!
>> LGTM, except for a few nits below and the single question.
>>>  
>>>> From: Mike Pall <mike>
>>>> 
>>>> Reported by Yichun Zhang.
>>>> 
>>>> (cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)
>>>> 
>>>> The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered
>>>> as 64 bit address. On GC64 mode it may lead to the following assembly:
>>> Typo: s/as 64 bit/a 64-bit/
> 
> Fixed, thanks!
> 
>>>> | mov eax, edi
>>>> so, high 32 bits of the reference are lost.
>>> Typo: s/high/the high/
>>>> 
>>>> This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`
>>>> 64 bit long. Now the resulting assembly is the following:
>>>> | mov rax, rdi
> 
> Fixed, thanks!
> 
> Branch is force-pushed.
> 
>>>> 
>>>> False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`
>>>> already initialized as 0.
>>>> 
>>>> False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,
>>>> <src/lj_record.c> is OK, since `REF_NIL` is the last reference before
>>>> `REF_BASE` and this iteration of a cycle is still the last one.
>>>> 
>>>> Sergey Kaplun:
>>>> * added the description and the test for the problem
>>>> 
>>>> Part of tarantool/tarantool#8516
>>>> ---
>>>> 
>>>> Branch:  https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l
>>>> Related issues:
>>>> *  https://github.com/openresty/lua-resty-core/issues/144
>>>> *  https://github.com/tarantool/tarantool/issues/8516
>>>> PR:  https://github.com/tarantool/tarantool/pull/8553
>>>> ML:  https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode
>>>> 
> 
> <snipped>
> 
>>>> +local global_env
>>>> +local _
>>>> +for i = 1, 4 do
>>>> + -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).
>>>> + global_env = getfenv(0)
>>>> + -- Need to reuse the register, to cause emitting of `mov`
>>>> + -- instruction (see `ra_left()` in <src/lj_asm.c>).
>>>> + _ = tostring(i)
>>>> +end
>>>> +
>>>> +test:ok(global_env == getfenv(0), 'IR_LREF assembling correctness')
>>>> +
>>>> +os.exit(test:check() and 0 or 1)
>>> Neither this test case, nor the original one from OpenResty fail before the patch on OSX/ARM64.
>>> Is it expected behavior or not?
> 
> Yes, I think that non x86_64 arches are unaffected, since they use
> `ra_leftov()` instead.
> 
>>> On x86 GC64 it behaves as expected though.
>>>> --
>>>> 2.34.1
>>> --
>>> Best regards,
>>> Maxim Kokryashkin
>>>  
> 
> -- 
> Best regards,
> Sergey Kaplun


[-- Attachment #2: Type: text/html, Size: 17256 bytes --]

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Sergey Kaplun via Tarantool-patches]

Hi, Sergos!
Thanks for the review!

On 03.05.23, sergos wrote:
> Hi!
> 
> Thanks for the patch!
> 
> Since test is x86_64 only - can we put an explicit skipcond then?

We have two sides of the same coin here:

There is no bug for arm64, but some source code for this IR still
exists. Yes, we don't see any influence of the patch on arm64.

But, OTOH, we sure that there will be no regression in the future. As
far as changed code in the patch is platform independent, I prefer to
avoid skipcond here.

So, ignoring for now.

> 
> Otherwise LGTM.
> 
> Sergos.
> 

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]


Hi!
Thanks for the fixes!
LGTM
--
Best regards,
Maxim Kokryashkin
 
 
> 
>>Hi, Max!
>>Thanks for the review!
>>
>>On 18.04.23, Maxim Kokryashkin wrote:
>>>
>>> Hi, Sergey!
>>> Thanks for the patch!
>>> LGTM, except for a few nits below and the single question.
>>> > 
>>> >>From: Mike Pall <mike>
>>> >>
>>> >>Reported by Yichun Zhang.
>>> >>
>>> >>(cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)
>>> >>
>>> >>The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered
>>> >>as 64 bit address. On GC64 mode it may lead to the following assembly:
>>> >Typo: s/as 64 bit/a 64-bit/
>>
>>Fixed, thanks!
>>
>>> >>| mov eax, edi
>>> >>so, high 32 bits of the reference are lost.
>>> >Typo: s/high/the high/
>>> >>
>>> >>This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`
>>> >>64 bit long. Now the resulting assembly is the following:
>>> >>| mov rax, rdi
>>
>>Fixed, thanks!
>>
>>Branch is force-pushed.
>>
>>> >>
>>> >>False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`
>>> >>already initialized as 0.
>>> >>
>>> >>False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,
>>> >><src/lj_record.c> is OK, since `REF_NIL` is the last reference before
>>> >>`REF_BASE` and this iteration of a cycle is still the last one.
>>> >>
>>> >>Sergey Kaplun:
>>> >>* added the description and the test for the problem
>>> >>
>>> >>Part of tarantool/tarantool#8516
>>> >>---
>>> >>
>>> >>Branch:  https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l
>>> >>Related issues:
>>> >>*  https://github.com/openresty/lua-resty-core/issues/144
>>> >>*  https://github.com/tarantool/tarantool/issues/8516
>>> >>PR:  https://github.com/tarantool/tarantool/pull/8553
>>> >>ML:  https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode
>>> >>
>>
>><snipped>
>>
>>> >>+local global_env
>>> >>+local _
>>> >>+for i = 1, 4 do
>>> >>+ -- Test `IR_LREF` assembling: using `ASMREF_L` (`REF_NIL`).
>>> >>+ global_env = getfenv(0)
>>> >>+ -- Need to reuse the register, to cause emitting of `mov`
>>> >>+ -- instruction (see `ra_left()` in <src/lj_asm.c>).
>>> >>+ _ = tostring(i)
>>> >>+end
>>> >>+
>>> >>+test:ok(global_env == getfenv(0), 'IR_LREF assembling correctness')
>>> >>+
>>> >>+os.exit(test:check() and 0 or 1)
>>> >Neither this test case, nor the original one from OpenResty fail before the patch on OSX/ARM64.
>>> >Is it expected behavior or not?
>>
>>Yes, I think that non x86_64 arches are unaffected, since they use
>>`ra_leftov()` instead.
>>
>>> >On x86 GC64 it behaves as expected though.
>>> >>--
>>> >>2.34.1
>>> >--
>>> >Best regards,
>>> >Maxim Kokryashkin
>>> > 
>>
>>--
>>Best regards,
>>Sergey Kaplun
> 

[-- Attachment #2: Type: text/html, Size: 4421 bytes --]

Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master, 2.11 and 2.10.

On 11.04.23, Sergey Kaplun via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> Reported by Yichun Zhang.
> 
> (cherry picked from commit 850f8c59d3d04a9847f21f32a6c36d8269b5b6b1)
> 
> The `ASMREF_L` reference is defined as `REF_NIL`, so it isn't considered
> as 64 bit address. On GC64 mode it may lead to the following assembly:
> | mov eax, edi
> so, high 32 bits of the reference are lost.
> 
> This patch adds `IRT_NIL` to `IRT_IS64` mask, to consider `ASMREF_L`
> 64 bit long. Now the resulting assembly is the following:
> | mov rax, rdi
> 
> False-positive `if` condition in <src/lj_asm.c> is OK, since `op12`
> already initialized as 0.
> 
> False-positive `if` condition in <src/lj_opt_sink.c>, <src/lj_opt_split.c>,
> <src/lj_record.c> is OK, since `REF_NIL` is the last reference before
> `REF_BASE` and this iteration of a cycle is still the last one.
> 
> Sergey Kaplun:
> * added the description and the test for the problem
> 
> Part of tarantool/tarantool#8516
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/or-144-gc64-asmref-l
> Related issues:
> * https://github.com/openresty/lua-resty-core/issues/144
> * https://github.com/tarantool/tarantool/issues/8516
> PR: https://github.com/tarantool/tarantool/pull/8553
> ML: https://www.freelists.org/post/luajit/Consistent-SEGV-on-x64-with-the-latest-LuaJIT-v21-GC64-mode
> 
> Don't restrict test case by GC64 mode, because want to test `IR_LREF`
> for any mode. Keep GC64 in the test name, to be clear where expect the
> SegFault.
> 
>  src/lj_asm.c                                  |  1 +
>  src/lj_ir.h                                   |  4 ++-
>  src/lj_opt_sink.c                             |  1 +
>  .../or-144-gc64-asmref-l.test.lua             | 28 +++++++++++++++++++
>  4 files changed, 33 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/or-144-gc64-asmref-l.test.lua
> 

<snipped>

> -- 
> 2.34.1
> 

-- 
Best regards,
IM