Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
@ 2023-04-11 20:36 Sergey Kaplun via Tarantool-patches
  2023-04-18 16:33 ` Maxim Kokryashkin via Tarantool-patches
  2023-05-25  6:16 ` Igor Munkin via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-04-11 20:36 UTC (permalink / raw)
  To: Sergey Ostanevich, Maxim Kokryashkin; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-04-11 20:36 [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit Sergey Kaplun via Tarantool-patches
@ 2023-04-18 16:33 ` Maxim Kokryashkin via Tarantool-patches
  2023-05-02  8:13   ` Sergey Kaplun via Tarantool-patches
  2023-05-25  6:16 ` Igor Munkin via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-04-18 16:33 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-04-18 16:33 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-02  8:13   ` Sergey Kaplun via Tarantool-patches
  2023-05-03  8:32     ` sergos via Tarantool-patches
  2023-05-03  8:56     ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-02  8:13 UTC (permalink / raw)
  To: Maxim Kokryashkin; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-05-02  8:13   ` Sergey Kaplun via Tarantool-patches
@ 2023-05-03  8:32     ` sergos via Tarantool-patches
  2023-05-03  8:40       ` Sergey Kaplun via Tarantool-patches
  2023-05-03  8:56     ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: sergos via Tarantool-patches @ 2023-05-03  8:32 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-05-03  8:32     ` sergos via Tarantool-patches
@ 2023-05-03  8:40       ` Sergey Kaplun via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-03  8:40 UTC (permalink / raw)
  To: sergos; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches]  [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-05-02  8:13   ` Sergey Kaplun via Tarantool-patches
  2023-05-03  8:32     ` sergos via Tarantool-patches
@ 2023-05-03  8:56     ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-03  8:56 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit.
  2023-04-11 20:36 [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit Sergey Kaplun via Tarantool-patches
  2023-04-18 16:33 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-25  6:16 ` Igor Munkin via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-05-25  6:16 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-05-25  6:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-11 20:36 [Tarantool-patches] [PATCH luajit] LJ_GC64: Make ASMREF_L references 64 bit Sergey Kaplun via Tarantool-patches
2023-04-18 16:33 ` Maxim Kokryashkin via Tarantool-patches
2023-05-02  8:13   ` Sergey Kaplun via Tarantool-patches
2023-05-03  8:32     ` sergos via Tarantool-patches
2023-05-03  8:40       ` Sergey Kaplun via Tarantool-patches
2023-05-03  8:56     ` Maxim Kokryashkin via Tarantool-patches
2023-05-25  6:16 ` Igor Munkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox