[Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by Sergey Kaplun.

(cherry-picked from commit d133d67c881f363f0b5584ebd21a965eb3435aa1)

This patch is the follow-up for the commit
1672bdc0ffbee9f32fadb6943909d6af7eb9a7b1 ("x64: Fix __call metamethod
return dispatch."). This patch uses the incorrect macro x64 (which is
undefined in dasm) instead of X64. `KBASE` (r15d) is still compared
against `BASE` (edx) instead of `KBASEa` (r15) against rdx.

This patch fixes the typo, so the correct registers are chosen.

Sergey Kaplun:
* added the description for the problem

Part of tarantool/tarantool#9145
---

The test is omitted for the same reason as it was omitted in the 1672bdc
commit: it is too hard (or even impossible) to create the test case.

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1110-x64-return-dispatch
Tarantool PR: https://github.com/tarantool/tarantool/pull/9335
Relate issues and PRs:
* https://github.com/LuaJIT/LuaJIT/pull/636
* https://github.com/LuaJIT/LuaJIT/issues/1110
* https://github.com/tarantool/tarantool/issues/9145

 src/vm_x86.dasc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index 9fa9a3f7..6a353796 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -1451,7 +1451,7 @@ static void build_subroutines(BuildCtx *ctx)
   |  mov LFUNC:RB, [RA-8]
   |  add NARGS:RD, 1
   |  // This is fragile. L->base must not move, KBASE must always be defined.
-  |.if x64
+  |.if X64
   |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
   |.else
   |  cmp KBASE, BASE			// Continue with CALLT if flag set.
-- 
2.42.0

Re: [Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the patch! LGTM as obvious, except the single nit regarding
the commit message.

On 07.11.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> (cherry-picked from commit d133d67c881f363f0b5584ebd21a965eb3435aa1)
> 
> This patch is the follow-up for the commit
> 1672bdc0ffbee9f32fadb6943909d6af7eb9a7b1 ("x64: Fix __call metamethod
> return dispatch."). This patch uses the incorrect macro x64 (which is

I don't get which one of the aforementioned patches is "this": *this*
one fixing the issue, or *that* one, that doesn't.

> undefined in dasm) instead of X64. `KBASE` (r15d) is still compared
> against `BASE` (edx) instead of `KBASEa` (r15) against rdx.
> 
> This patch fixes the typo, so the correct registers are chosen.

Side note: I hope it definitely is, but we still have no tests for it.

> 
> Sergey Kaplun:
> * added the description for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> 
> The test is omitted for the same reason as it was omitted in the 1672bdc
> commit: it is too hard (or even impossible) to create the test case.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1110-x64-return-dispatch
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9335
> Relate issues and PRs:
> * https://github.com/LuaJIT/LuaJIT/pull/636
> * https://github.com/LuaJIT/LuaJIT/issues/1110
> * https://github.com/tarantool/tarantool/issues/9145
> 
>  src/vm_x86.dasc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index 9fa9a3f7..6a353796 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -1451,7 +1451,7 @@ static void build_subroutines(BuildCtx *ctx)
>    |  mov LFUNC:RB, [RA-8]
>    |  add NARGS:RD, 1
>    |  // This is fragile. L->base must not move, KBASE must always be defined.
> -  |.if x64
> +  |.if X64

Bonjour.

>    |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
>    |.else
>    |  cmp KBASE, BASE			// Continue with CALLT if flag set.
> -- 
> 2.42.0
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[Sergey Kaplun via Tarantool-patches]

Hi, Igor!
Thanks for the review!
Fixed the nit and force-pushed the branch.

On 07.11.23, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! LGTM as obvious, except the single nit regarding
> the commit message.
> 
> On 07.11.23, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> > 
> > Reported by Sergey Kaplun.
> > 
> > (cherry-picked from commit d133d67c881f363f0b5584ebd21a965eb3435aa1)
> > 
> > This patch is the follow-up for the commit
> > 1672bdc0ffbee9f32fadb6943909d6af7eb9a7b1 ("x64: Fix __call metamethod
> > return dispatch."). This patch uses the incorrect macro x64 (which is
> 
> I don't get which one of the aforementioned patches is "this": *this*
> one fixing the issue, or *that* one, that doesn't.

Fixed to "That", as you suggested. Force-pushed the branch.

> 

<snipped>

> > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> > index 9fa9a3f7..6a353796 100644
> > --- a/src/vm_x86.dasc
> > +++ b/src/vm_x86.dasc
> > @@ -1451,7 +1451,7 @@ static void build_subroutines(BuildCtx *ctx)
> >    |  mov LFUNC:RB, [RA-8]
> >    |  add NARGS:RD, 1
> >    |  // This is fragile. L->base must not move, KBASE must always be defined.
> > -  |.if x64
> > +  |.if X64
> 
> Bonjour.

Aah! Et bonjour mes nouveaux (espérons-le) amis !

> 
> >    |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
> >    |.else
> >    |  cmp KBASE, BASE			// Continue with CALLT if flag set.
> > -- 
> > 2.42.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[Maxim Kokryashkin via Tarantool-patches]

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


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Вторник, 7 ноября 2023, 14:15 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
> 
>Hi, Igor!
>Thanks for the review!
>Fixed the nit and force-pushed the branch.
>
>On 07.11.23, Igor Munkin wrote:
>> Sergey,
>>
>> Thanks for the patch! LGTM as obvious, except the single nit regarding
>> the commit message.
>>
>> On 07.11.23, Sergey Kaplun wrote:
>> > From: Mike Pall <mike>
>> >
>> > Reported by Sergey Kaplun.
>> >
>> > (cherry-picked from commit d133d67c881f363f0b5584ebd21a965eb3435aa1)
>> >
>> > This patch is the follow-up for the commit
>> > 1672bdc0ffbee9f32fadb6943909d6af7eb9a7b1 ("x64: Fix __call metamethod
>> > return dispatch."). This patch uses the incorrect macro x64 (which is
>>
>> I don't get which one of the aforementioned patches is "this": *this*
>> one fixing the issue, or *that* one, that doesn't.
>
>Fixed to "That", as you suggested. Force-pushed the branch.
>
>>
>
><snipped>
>
>> > diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
>> > index 9fa9a3f7..6a353796 100644
>> > --- a/src/vm_x86.dasc
>> > +++ b/src/vm_x86.dasc
>> > @@ -1451,7 +1451,7 @@ static void build_subroutines(BuildCtx *ctx)
>> > | mov LFUNC:RB, [RA-8]
>> > | add NARGS:RD, 1
>> > | // This is fragile. L->base must not move, KBASE must always be defined.
>> > - |.if x64
>> > + |.if X64
>>
>> Bonjour.
>
>Aah! Et bonjour mes nouveaux (espérons-le) amis !
>
>>
>> > | cmp KBASEa, rdx // Continue with CALLT if flag set.
>> > |.else
>> > | cmp KBASE, BASE // Continue with CALLT if flag set.
>> > --
>> > 2.42.0
>> >
>>
>> --
>> Best regards,
>> IM
>
>--
>Best regards,
>Sergey Kaplun
 

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

Re: [Tarantool-patches] [PATCH luajit] x64: Properly fix __call metamethod return dispatch.

[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, release/2.11 and
release/2.10.

On 07.11.23, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Reported by Sergey Kaplun.
> 
> (cherry-picked from commit d133d67c881f363f0b5584ebd21a965eb3435aa1)
> 
> This patch is the follow-up for the commit
> 1672bdc0ffbee9f32fadb6943909d6af7eb9a7b1 ("x64: Fix __call metamethod
> return dispatch."). This patch uses the incorrect macro x64 (which is
> undefined in dasm) instead of X64. `KBASE` (r15d) is still compared
> against `BASE` (edx) instead of `KBASEa` (r15) against rdx.
> 
> This patch fixes the typo, so the correct registers are chosen.
> 
> Sergey Kaplun:
> * added the description for the problem
> 
> Part of tarantool/tarantool#9145
> ---
> 
> The test is omitted for the same reason as it was omitted in the 1672bdc
> commit: it is too hard (or even impossible) to create the test case.
> 
> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1110-x64-return-dispatch
> Tarantool PR: https://github.com/tarantool/tarantool/pull/9335
> Relate issues and PRs:
> * https://github.com/LuaJIT/LuaJIT/pull/636
> * https://github.com/LuaJIT/LuaJIT/issues/1110
> * https://github.com/tarantool/tarantool/issues/9145
> 
>  src/vm_x86.dasc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

<snipped>

> -- 
> 2.42.0
> 

-- 
Best regards,
IM