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

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

[Igor Munkin]

From: Mike Pall <mike>

After linking new cframe to the chain KBASEa still stores the address of
the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
value (i.e. low 32 bits of the stored address) might be equal to the
current BASE address value so the execution takes the invalid path. Such
address clashing occurs only on x86_64 platform with disabled LJ_GC64,
so 64-bit registers have to be compared in x64 build.

NB: Though there is only 32-bit load to restore BASE value prior to the
comparison, the high 32 bits of RDX are reset to zeros, according to x86
long mode semantics.

Igor Munkin:
* backported the original patch to tarantool/luajit repo
* extended the original commit message with the rationale

For more info and explanation see LuaJIT/LuaJIT#636.

Relates to tarantool/tarantool#4518
Relates to tarantool/tarantool#4649

Signed-off-by: Igor Munkin <imun@tarantool.org>
---

Issues:
* https://github.com/tarantool/tarantool/issues/4518
* https://github.com/tarantool/tarantool/issues/4649
Branch:
* https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call

CI is kinda green, considering C6 EOL and the corresponding failures:
* https://gitlab.com/tarantool/tarantool/-/pipelines/225349795

@ChangeLog:
* Fixed address clashing occurring while __call metamethod dispatching
  (gh-4518, gh-4649).

Unfortunately, there is neither test nor reproducer for this failure, so
we'll know that the patch works only on production installations.

 src/vm_x86.dasc | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index 56bee14..d76fbe3 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -1374,7 +1374,11 @@ 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
+  |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
+  |.else
   |  cmp KBASE, BASE			// Continue with CALLT if flag set.
+  |.endif
   |  je ->BC_CALLT_Z
   |  mov BASE, RA
   |  ins_call				// Otherwise call resolved metamethod.
-- 
2.25.0

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

[Sergey Kaplun]

Hi, Igor!

Thanks for the patch!
LGTM except the single nit.

On 04.12.20, Igor Munkin wrote:
> From: Mike Pall <mike>
> 
> After linking new cframe to the chain KBASEa still stores the address of
> the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> value (i.e. low 32 bits of the stored address) might be equal to the
> current BASE address value so the execution takes the invalid path. Such
> address clashing occurs only on x86_64 platform with disabled LJ_GC64,

Typo: s/address/address parts/ (considering your message in PR).

> so 64-bit registers have to be compared in x64 build.
> 
> NB: Though there is only 32-bit load to restore BASE value prior to the
> comparison, the high 32 bits of RDX are reset to zeros, according to x86
> long mode semantics.
> 
> Igor Munkin:
> * backported the original patch to tarantool/luajit repo
> * extended the original commit message with the rationale
> 
> For more info and explanation see LuaJIT/LuaJIT#636.
> 
> Relates to tarantool/tarantool#4518
> Relates to tarantool/tarantool#4649
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Issues:
> * https://github.com/tarantool/tarantool/issues/4518
> * https://github.com/tarantool/tarantool/issues/4649
> Branch:
> * https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call
> 
> CI is kinda green, considering C6 EOL and the corresponding failures:
> * https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> 
> @ChangeLog:
> * Fixed address clashing occurring while __call metamethod dispatching
>   (gh-4518, gh-4649).
> 
> Unfortunately, there is neither test nor reproducer for this failure, so
> we'll know that the patch works only on production installations.
> 
>  src/vm_x86.dasc | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index 56bee14..d76fbe3 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -1374,7 +1374,11 @@ 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
> +  |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
> +  |.else
>    |  cmp KBASE, BASE			// Continue with CALLT if flag set.
> +  |.endif
>    |  je ->BC_CALLT_Z
>    |  mov BASE, RA
>    |  ins_call				// Otherwise call resolved metamethod.
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

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

[Igor Munkin]

Sergey,

Thanks for your review!

On 04.12.20, Sergey Kaplun wrote:
> Hi, Igor!
> 
> Thanks for the patch!
> LGTM except the single nit.

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> On 04.12.20, Igor Munkin wrote:
> > From: Mike Pall <mike>
> > 
> > After linking new cframe to the chain KBASEa still stores the address of
> > the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> > value (i.e. low 32 bits of the stored address) might be equal to the
> > current BASE address value so the execution takes the invalid path. Such
> > address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> 
> Typo: s/address/address parts/ (considering your message in PR).

Thanks, fixed.

> 
> > so 64-bit registers have to be compared in x64 build.
> > 
> > NB: Though there is only 32-bit load to restore BASE value prior to the
> > comparison, the high 32 bits of RDX are reset to zeros, according to x86
> > long mode semantics.
> > 
> > Igor Munkin:
> > * backported the original patch to tarantool/luajit repo
> > * extended the original commit message with the rationale
> > 
> > For more info and explanation see LuaJIT/LuaJIT#636.
> > 
> > Relates to tarantool/tarantool#4518
> > Relates to tarantool/tarantool#4649
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 

<snipped>

> > 
> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

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

[Sergey Ostanevich]

Hi! 
Thanks for the patch, LGTM.

Sergos


> On 4 Dec 2020, at 16:42, Igor Munkin <imun@tarantool.org> wrote:
> 
> From: Mike Pall <mike>
> 
> After linking new cframe to the chain KBASEa still stores the address of
> the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> value (i.e. low 32 bits of the stored address) might be equal to the
> current BASE address value so the execution takes the invalid path. Such
> address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> so 64-bit registers have to be compared in x64 build.
> 
> NB: Though there is only 32-bit load to restore BASE value prior to the
> comparison, the high 32 bits of RDX are reset to zeros, according to x86
> long mode semantics.
> 
> Igor Munkin:
> * backported the original patch to tarantool/luajit repo
> * extended the original commit message with the rationale
> 
> For more info and explanation see LuaJIT/LuaJIT#636.
> 
> Relates to tarantool/tarantool#4518
> Relates to tarantool/tarantool#4649
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Issues:
> * https://github.com/tarantool/tarantool/issues/4518
> * https://github.com/tarantool/tarantool/issues/4649
> Branch:
> * https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call
> 
> CI is kinda green, considering C6 EOL and the corresponding failures:
> * https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> 
> @ChangeLog:
> * Fixed address clashing occurring while __call metamethod dispatching
>  (gh-4518, gh-4649).
> 
> Unfortunately, there is neither test nor reproducer for this failure, so
> we'll know that the patch works only on production installations.
> 
> src/vm_x86.dasc | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
> index 56bee14..d76fbe3 100644
> --- a/src/vm_x86.dasc
> +++ b/src/vm_x86.dasc
> @@ -1374,7 +1374,11 @@ 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
> +  |  cmp KBASEa, rdx			// Continue with CALLT if flag set.
> +  |.else
>   |  cmp KBASE, BASE			// Continue with CALLT if flag set.
> +  |.endif
>   |  je ->BC_CALLT_Z
>   |  mov BASE, RA
>   |  ins_call				// Otherwise call resolved metamethod.
> -- 
> 2.25.0
> 

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

[Igor Munkin]

Sergos,

Thanks for your review!

On 04.12.20, Sergey Ostanevich wrote:
> Hi! 
> Thanks for the patch, LGTM.

Added your tag:
| Reviewed-by: Sergey Ostanevich <sergos@tarantool.org>

> 
> Sergos
> 

<snipped>

> 

-- 
Best regards,
IM

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

[Igor Munkin]

Sasha,

Could you please confirm whether CI is OK?

On 04.12.20, Igor Munkin wrote:
> From: Mike Pall <mike>
> 
> After linking new cframe to the chain KBASEa still stores the address of
> the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> value (i.e. low 32 bits of the stored address) might be equal to the
> current BASE address value so the execution takes the invalid path. Such
> address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> so 64-bit registers have to be compared in x64 build.
> 
> NB: Though there is only 32-bit load to restore BASE value prior to the
> comparison, the high 32 bits of RDX are reset to zeros, according to x86
> long mode semantics.
> 
> Igor Munkin:
> * backported the original patch to tarantool/luajit repo
> * extended the original commit message with the rationale
> 
> For more info and explanation see LuaJIT/LuaJIT#636.
> 
> Relates to tarantool/tarantool#4518
> Relates to tarantool/tarantool#4649
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Issues:
> * https://github.com/tarantool/tarantool/issues/4518
> * https://github.com/tarantool/tarantool/issues/4649
> Branch:
> * https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call
> 
> CI is kinda green, considering C6 EOL and the corresponding failures:
> * https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> 
> @ChangeLog:
> * Fixed address clashing occurring while __call metamethod dispatching
>   (gh-4518, gh-4649).
> 
> Unfortunately, there is neither test nor reproducer for this failure, so
> we'll know that the patch works only on production installations.
> 

<snipped>

> 

-- 
Best regards,
IM

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

[Alexander V. Tikhonov]

Hi Igor, thanks for the patch, as I see no new degradations found [1],
patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/225349795

On Fri, Dec 04, 2020 at 07:24:41PM +0300, Igor Munkin wrote:
> Sasha,
> 
> Could you please confirm whether CI is OK?
> 
> On 04.12.20, Igor Munkin wrote:
> > From: Mike Pall <mike>
> > 
> > After linking new cframe to the chain KBASEa still stores the address of
> > the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> > value (i.e. low 32 bits of the stored address) might be equal to the
> > current BASE address value so the execution takes the invalid path. Such
> > address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> > so 64-bit registers have to be compared in x64 build.
> > 
> > NB: Though there is only 32-bit load to restore BASE value prior to the
> > comparison, the high 32 bits of RDX are reset to zeros, according to x86
> > long mode semantics.
> > 
> > Igor Munkin:
> > * backported the original patch to tarantool/luajit repo
> > * extended the original commit message with the rationale
> > 
> > For more info and explanation see LuaJIT/LuaJIT#636.
> > 
> > Relates to tarantool/tarantool#4518
> > Relates to tarantool/tarantool#4649
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Issues:
> > * https://github.com/tarantool/tarantool/issues/4518
> > * https://github.com/tarantool/tarantool/issues/4649
> > Branch:
> > * https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call
> > 
> > CI is kinda green, considering C6 EOL and the corresponding failures:
> > * https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> > 
> > @ChangeLog:
> > * Fixed address clashing occurring while __call metamethod dispatching
> >   (gh-4518, gh-4649).
> > 
> > Unfortunately, there is neither test nor reproducer for this failure, so
> > we'll know that the patch works only on production installations.
> > 
> 
> <snipped>
> 
> > 
> 
> -- 
> Best regards,
> IM

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

[Igor Munkin]

Sasha,

On 04.12.20, Alexander V. Tikhonov wrote:
> Hi Igor, thanks for the patch, as I see no new degradations found [1],
> patch LGTM.

Great, thanks!

Kirill, please proceed with the patch.

> 
> [1] - https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> 
> On Fri, Dec 04, 2020 at 07:24:41PM +0300, Igor Munkin wrote:
> > Sasha,
> > 
> > Could you please confirm whether CI is OK?
> > 
> > On 04.12.20, Igor Munkin wrote:
> > > From: Mike Pall <mike>
> > > 
> > > After linking new cframe to the chain KBASEa still stores the address of
> > > the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> > > value (i.e. low 32 bits of the stored address) might be equal to the
> > > current BASE address value so the execution takes the invalid path. Such
> > > address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> > > so 64-bit registers have to be compared in x64 build.
> > > 
> > > NB: Though there is only 32-bit load to restore BASE value prior to the
> > > comparison, the high 32 bits of RDX are reset to zeros, according to x86
> > > long mode semantics.
> > > 
> > > Igor Munkin:
> > > * backported the original patch to tarantool/luajit repo
> > > * extended the original commit message with the rationale
> > > 
> > > For more info and explanation see LuaJIT/LuaJIT#636.
> > > 
> > > Relates to tarantool/tarantool#4518
> > > Relates to tarantool/tarantool#4649
> > > 
> > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > ---
> > > 
> > > Issues:
> > > * https://github.com/tarantool/tarantool/issues/4518
> > > * https://github.com/tarantool/tarantool/issues/4649
> > > Branch:
> > > * https://github.com/tarantool/luajit/tree/imun/gh-4518-cmp-64-bit-regs-in-vmeta-call
> > > 
> > > CI is kinda green, considering C6 EOL and the corresponding failures:
> > > * https://gitlab.com/tarantool/tarantool/-/pipelines/225349795
> > > 
> > > @ChangeLog:
> > > * Fixed address clashing occurring while __call metamethod dispatching
> > >   (gh-4518, gh-4649).
> > > 
> > > Unfortunately, there is neither test nor reproducer for this failure, so
> > > we'll know that the patch works only on production installations.
> > > 
> > 
> > <snipped>
> > 
> > > 
> > 
> > -- 
> > Best regards,
> > IM

-- 
Best regards,
IM

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

[Kirill Yukhin]

Hello,

On 04 Dec 16:42, Igor Munkin via Tarantool-patches wrote:
> From: Mike Pall <mike>
> 
> After linking new cframe to the chain KBASEa still stores the address of
> the previous one. If the execution proceeds to <lj_vmeta_call> KBASE
> value (i.e. low 32 bits of the stored address) might be equal to the
> current BASE address value so the execution takes the invalid path. Such
> address clashing occurs only on x86_64 platform with disabled LJ_GC64,
> so 64-bit registers have to be compared in x64 build.
> 
> NB: Though there is only 32-bit load to restore BASE value prior to the
> comparison, the high 32 bits of RDX are reset to zeros, according to x86
> long mode semantics.
> 
> Igor Munkin:
> * backported the original patch to tarantool/luajit repo
> * extended the original commit message with the rationale
> 
> For more info and explanation see LuaJIT/LuaJIT#636.
> 
> Relates to tarantool/tarantool#4518
> Relates to tarantool/tarantool#4649
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>

I've checked your patch into Tarantool's luakit release branches and
bumped a new version in 1.10, 2.5, 2.6 and master.

--
Regards, Kirill Yukhin