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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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