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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- 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 --]