Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
@ 2020-10-14 13:53 Igor Munkin
  2020-10-14 18:18 ` Sergey Ostanevich
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Igor Munkin @ 2020-10-14 13:53 UTC (permalink / raw)
  To: Sergey Ostanevich, Kirill Yukhin, Sergey Kaplun; +Cc: tarantool-patches

This patch fixes the regression introduced in scope of
5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
platform metrics'). As a result of the patch <cdatanum> displacement is
misencoded when GC64 mode is enabled.

In X86 long mode 32-bit displacement is encoded either via SIB byte or
is addressed relatively to RIP register value. The first approach is
used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
doesn't work for 64-bit ones. As a result all addresses to GG_State
contents to be "hardcoded" on the trace are encoded relatively to
RID_DISPATCH register (i.e. callee-safe R14 register) containing global
dispatch table. For this purpose this register is not used by the JIT
register allocator in GC64 build and not spoiled throughout LuaJIT VM
cycle (and therefore trace execution).

NB: Since R14 is the additional GRP, the <add> instruction ought to be
REX-prefixed.

Follows up tarantool/tarantool#5187

Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Signed-off-by: Igor Munkin <imun@tarantool.org>
---

Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64

Unforunately, CI is red, but those failures relates to the known build
issues. Nevertheless I tested the patch manually on tntmac04 and faced
no failures.

 src/lj_asm_x86.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
index 959fc2d..767bf6f 100644
--- a/src/lj_asm_x86.h
+++ b/src/lj_asm_x86.h
@@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
 
   /* Increment cdatanum counter by address directly. */
   emit_i8(as, 1);
+#if LJ_GC64
+  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
+	    dispofs(as, &J2G(as->J)->gc.cdatanum));
+#else
   emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
 	    ptr2addr(&J2G(as->J)->gc.cdatanum));
+#endif
   /* Combine initialization of marked, gct and ctypeid. */
   emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
   emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
-- 
2.25.0

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 13:53 [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86 Igor Munkin
@ 2020-10-14 18:18 ` Sergey Ostanevich
  2020-10-14 18:31   ` Igor Munkin
  2020-10-14 19:04 ` Sergey Kaplun
  2020-10-15  8:41 ` Kirill Yukhin
  2 siblings, 1 reply; 8+ messages in thread
From: Sergey Ostanevich @ 2020-10-14 18:18 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Hi!

Thanks for the patch, I got one question below.

Regards,
Sergos

On 14 окт 16:53, Igor Munkin wrote:
> This patch fixes the regression introduced in scope of
> 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> platform metrics'). As a result of the patch <cdatanum> displacement is
> misencoded when GC64 mode is enabled.
> 
> In X86 long mode 32-bit displacement is encoded either via SIB byte or
> is addressed relatively to RIP register value. The first approach is
> used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> doesn't work for 64-bit ones. As a result all addresses to GG_State
> contents to be "hardcoded" on the trace are encoded relatively to
> RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> dispatch table. For this purpose this register is not used by the JIT
> register allocator in GC64 build and not spoiled throughout LuaJIT VM
> cycle (and therefore trace execution).
> 
> NB: Since R14 is the additional GRP, the <add> instruction ought to be
> REX-prefixed.
> 
> Follows up tarantool/tarantool#5187
> 
> Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> 
> Unforunately, CI is red, but those failures relates to the known build
> issues. Nevertheless I tested the patch manually on tntmac04 and faced
> no failures.
> 
>  src/lj_asm_x86.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 959fc2d..767bf6f 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h
> @@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
>  
>    /* Increment cdatanum counter by address directly. */
>    emit_i8(as, 1);
> +#if LJ_GC64
> +  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
> +	    dispofs(as, &J2G(as->J)->gc.cdatanum));

Should we cast the disp to 32bit? Here
https://wiki.osdev.org/X86-64_Instruction_Encoding#Displacement
I see only a disp32. 

> +#else
>    emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
>  	    ptr2addr(&J2G(as->J)->gc.cdatanum));
> +#endif
>    /* Combine initialization of marked, gct and ctypeid. */
>    emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
>    emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
> -- 
> 2.25.0
> 

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 18:18 ` Sergey Ostanevich
@ 2020-10-14 18:31   ` Igor Munkin
  2020-10-14 20:11     ` Sergey Ostanevich
  0 siblings, 1 reply; 8+ messages in thread
From: Igor Munkin @ 2020-10-14 18:31 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Sergos,

Thanks for you review!

On 14.10.20, Sergey Ostanevich wrote:
> Hi!
> 
> Thanks for the patch, I got one question below.
> 
> Regards,
> Sergos
> 
> On 14 окт 16:53, Igor Munkin wrote:
> > This patch fixes the regression introduced in scope of
> > 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> > platform metrics'). As a result of the patch <cdatanum> displacement is
> > misencoded when GC64 mode is enabled.
> > 
> > In X86 long mode 32-bit displacement is encoded either via SIB byte or
> > is addressed relatively to RIP register value. The first approach is
> > used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> > doesn't work for 64-bit ones. As a result all addresses to GG_State
> > contents to be "hardcoded" on the trace are encoded relatively to
> > RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> > dispatch table. For this purpose this register is not used by the JIT
> > register allocator in GC64 build and not spoiled throughout LuaJIT VM
> > cycle (and therefore trace execution).
> > 
> > NB: Since R14 is the additional GRP, the <add> instruction ought to be
> > REX-prefixed.
> > 
> > Follows up tarantool/tarantool#5187
> > 
> > Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> > 
> > Unforunately, CI is red, but those failures relates to the known build
> > issues. Nevertheless I tested the patch manually on tntmac04 and faced
> > no failures.
> > 
> >  src/lj_asm_x86.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> > index 959fc2d..767bf6f 100644
> > --- a/src/lj_asm_x86.h
> > +++ b/src/lj_asm_x86.h
> > @@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
> >  
> >    /* Increment cdatanum counter by address directly. */
> >    emit_i8(as, 1);
> > +#if LJ_GC64
> > +  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
> > +	    dispofs(as, &J2G(as->J)->gc.cdatanum));
> 
> Should we cast the disp to 32bit? Here

IIRC, in function calls, the arguments are converted to the types of the
corresponding parameters. <ofs> parameter in <emit_rmro> is int32_t
type, so I guess an explicit cast is not obligatory here, isn't it?

> https://wiki.osdev.org/X86-64_Instruction_Encoding#Displacement
> I see only a disp32. 

However, as you've already mentioned offline the *valid* dispofs values
fit 32-bit integers since the size of GG_State equals to 6344 bytes.

I surmise, these explicit casts around relate to the old dark times when
various compilers were not so good, so Mike had to add such casts
everywhere. I checked the machine code generated by GCC on my machine
and see no difference between two versions: with or without the cast.

> 
> > +#else
> >    emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
> >  	    ptr2addr(&J2G(as->J)->gc.cdatanum));
> > +#endif
> >    /* Combine initialization of marked, gct and ctypeid. */
> >    emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
> >    emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
> > -- 
> > 2.25.0
> > 

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 13:53 [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86 Igor Munkin
  2020-10-14 18:18 ` Sergey Ostanevich
@ 2020-10-14 19:04 ` Sergey Kaplun
  2020-10-14 19:22   ` Igor Munkin
  2020-10-15  8:41 ` Kirill Yukhin
  2 siblings, 1 reply; 8+ messages in thread
From: Sergey Kaplun @ 2020-10-14 19:04 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Hi!

Thanks for the patch!
I have only one nitpick/question regarding to commit message.

Otherwise, LGTM.

On 14.10.20, Igor Munkin wrote:
> This patch fixes the regression introduced in scope of
> 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> platform metrics'). As a result of the patch <cdatanum> displacement is
> misencoded when GC64 mode is enabled.
> 
> In X86 long mode 32-bit displacement is encoded either via SIB byte or
> is addressed relatively to RIP register value. The first approach is
> used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> doesn't work for 64-bit ones. As a result all addresses to GG_State
> contents to be "hardcoded" on the trace are encoded relatively to
> RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> dispatch table. For this purpose this register is not used by the JIT
> register allocator in GC64 build and not spoiled throughout LuaJIT VM
> cycle (and therefore trace execution).
> 
> NB: Since R14 is the additional GRP, the <add> instruction ought to be
> REX-prefixed.

Nit: Also REX prefix is needed to specify 64-bit operand size, isn't
it?

> 
> Follows up tarantool/tarantool#5187
> 
> Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> 
> Unforunately, CI is red, but those failures relates to the known build
> issues. Nevertheless I tested the patch manually on tntmac04 and faced
> no failures.
> 
>  src/lj_asm_x86.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 959fc2d..767bf6f 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h
> @@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
>  
>    /* Increment cdatanum counter by address directly. */
>    emit_i8(as, 1);
> +#if LJ_GC64
> +  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
> +	    dispofs(as, &J2G(as->J)->gc.cdatanum));
> +#else
>    emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
>  	    ptr2addr(&J2G(as->J)->gc.cdatanum));
> +#endif
>    /* Combine initialization of marked, gct and ctypeid. */
>    emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
>    emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
> -- 
> 2.25.0
> 

-- 
Best regards,
Sergey Kaplun

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 19:04 ` Sergey Kaplun
@ 2020-10-14 19:22   ` Igor Munkin
  0 siblings, 0 replies; 8+ messages in thread
From: Igor Munkin @ 2020-10-14 19:22 UTC (permalink / raw)
  To: Sergey Kaplun; +Cc: tarantool-patches

Sergey,

Thanks for your review!

On 14.10.20, Sergey Kaplun wrote:
> Hi!
> 
> Thanks for the patch!
> I have only one nitpick/question regarding to commit message.
> 
> Otherwise, LGTM.

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

> 
> On 14.10.20, Igor Munkin wrote:
> > This patch fixes the regression introduced in scope of
> > 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> > platform metrics'). As a result of the patch <cdatanum> displacement is
> > misencoded when GC64 mode is enabled.
> > 
> > In X86 long mode 32-bit displacement is encoded either via SIB byte or
> > is addressed relatively to RIP register value. The first approach is
> > used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> > doesn't work for 64-bit ones. As a result all addresses to GG_State
> > contents to be "hardcoded" on the trace are encoded relatively to
> > RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> > dispatch table. For this purpose this register is not used by the JIT
> > register allocator in GC64 build and not spoiled throughout LuaJIT VM
> > cycle (and therefore trace execution).
> > 
> > NB: Since R14 is the additional GRP, the <add> instruction ought to be
> > REX-prefixed.
> 
> Nit: Also REX prefix is needed to specify 64-bit operand size, isn't
> it?

Nice catch! Totally missed W-bit. Adjusted this line the following way:
| NB: Since R14 is the additional GRP and default <add> operand size is
| 32-bit (but 64-bit is used), the instruction ought to be REX-prefixed.

> 
> > 
> > Follows up tarantool/tarantool#5187
> > 
> > Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---
> > 
> > Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> > 

<snipped>

> 
> -- 
> Best regards,
> Sergey Kaplun

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 18:31   ` Igor Munkin
@ 2020-10-14 20:11     ` Sergey Ostanevich
  2020-10-14 20:13       ` Igor Munkin
  0 siblings, 1 reply; 8+ messages in thread
From: Sergey Ostanevich @ 2020-10-14 20:11 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches



Thanks! 

LGTM.


On 14 окт 21:31, Igor Munkin wrote:
> Sergos,
> 
> Thanks for you review!
> 
> On 14.10.20, Sergey Ostanevich wrote:
> > Hi!
> > 
> > Thanks for the patch, I got one question below.
> > 
> > Regards,
> > Sergos
> > 
> > On 14 окт 16:53, Igor Munkin wrote:
> > > This patch fixes the regression introduced in scope of
> > > 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> > > platform metrics'). As a result of the patch <cdatanum> displacement is
> > > misencoded when GC64 mode is enabled.
> > > 
> > > In X86 long mode 32-bit displacement is encoded either via SIB byte or
> > > is addressed relatively to RIP register value. The first approach is
> > > used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> > > doesn't work for 64-bit ones. As a result all addresses to GG_State
> > > contents to be "hardcoded" on the trace are encoded relatively to
> > > RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> > > dispatch table. For this purpose this register is not used by the JIT
> > > register allocator in GC64 build and not spoiled throughout LuaJIT VM
> > > cycle (and therefore trace execution).
> > > 
> > > NB: Since R14 is the additional GRP, the <add> instruction ought to be
> > > REX-prefixed.
> > > 
> > > Follows up tarantool/tarantool#5187
> > > 
> > > Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > ---
> > > 
> > > Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> > > 
> > > Unforunately, CI is red, but those failures relates to the known build
> > > issues. Nevertheless I tested the patch manually on tntmac04 and faced
> > > no failures.
> > > 
> > >  src/lj_asm_x86.h | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> > > index 959fc2d..767bf6f 100644
> > > --- a/src/lj_asm_x86.h
> > > +++ b/src/lj_asm_x86.h
> > > @@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
> > >  
> > >    /* Increment cdatanum counter by address directly. */
> > >    emit_i8(as, 1);
> > > +#if LJ_GC64
> > > +  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
> > > +	    dispofs(as, &J2G(as->J)->gc.cdatanum));
> > 
> > Should we cast the disp to 32bit? Here
> 
> IIRC, in function calls, the arguments are converted to the types of the
> corresponding parameters. <ofs> parameter in <emit_rmro> is int32_t
> type, so I guess an explicit cast is not obligatory here, isn't it?
> 
> > https://wiki.osdev.org/X86-64_Instruction_Encoding#Displacement
> > I see only a disp32. 
> 
> However, as you've already mentioned offline the *valid* dispofs values
> fit 32-bit integers since the size of GG_State equals to 6344 bytes.

I thought it should cover the difference from assembly placement to the
_G, but now I got it should cover only the one from dispatch table to 
your gc stuff. 

> 
> I surmise, these explicit casts around relate to the old dark times when
> various compilers were not so good, so Mike had to add such casts
> everywhere. I checked the machine code generated by GCC on my machine
> and see no difference between two versions: with or without the cast.
> 
> > 
> > > +#else
> > >    emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
> > >  	    ptr2addr(&J2G(as->J)->gc.cdatanum));
> > > +#endif
> > >    /* Combine initialization of marked, gct and ctypeid. */
> > >    emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
> > >    emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
> > > -- 
> > > 2.25.0
> > > 
> 
> -- 
> Best regards,
> IM

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 20:11     ` Sergey Ostanevich
@ 2020-10-14 20:13       ` Igor Munkin
  0 siblings, 0 replies; 8+ messages in thread
From: Igor Munkin @ 2020-10-14 20:13 UTC (permalink / raw)
  To: Sergey Ostanevich; +Cc: tarantool-patches

Sergos,

On 14.10.20, Sergey Ostanevich wrote:
> 
> 
> Thanks! 
> 
> LGTM.

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

> 
> 
> On 14 окт 21:31, Igor Munkin wrote:
> > Sergos,
> > 
> > Thanks for you review!
> > 
> > On 14.10.20, Sergey Ostanevich wrote:
> > > Hi!
> > > 
> > > Thanks for the patch, I got one question below.
> > > 
> > > Regards,
> > > Sergos
> > > 
> > > On 14 окт 16:53, Igor Munkin wrote:
> > > > This patch fixes the regression introduced in scope of
> > > > 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> > > > platform metrics'). As a result of the patch <cdatanum> displacement is
> > > > misencoded when GC64 mode is enabled.
> > > > 
> > > > In X86 long mode 32-bit displacement is encoded either via SIB byte or
> > > > is addressed relatively to RIP register value. The first approach is
> > > > used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> > > > doesn't work for 64-bit ones. As a result all addresses to GG_State
> > > > contents to be "hardcoded" on the trace are encoded relatively to
> > > > RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> > > > dispatch table. For this purpose this register is not used by the JIT
> > > > register allocator in GC64 build and not spoiled throughout LuaJIT VM
> > > > cycle (and therefore trace execution).
> > > > 
> > > > NB: Since R14 is the additional GRP, the <add> instruction ought to be
> > > > REX-prefixed.
> > > > 
> > > > Follows up tarantool/tarantool#5187
> > > > 
> > > > Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> > > > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > > > ---
> > > > 
> > > > Branch: https://github.com/tarantool/luajit/compare/imun/gh-5187-fix-disp-encoding-on-gc64
> > > > 
> > > > Unforunately, CI is red, but those failures relates to the known build
> > > > issues. Nevertheless I tested the patch manually on tntmac04 and faced
> > > > no failures.
> > > > 
> > > >  src/lj_asm_x86.h | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> > > > index 959fc2d..767bf6f 100644
> > > > --- a/src/lj_asm_x86.h
> > > > +++ b/src/lj_asm_x86.h
> > > > @@ -1837,8 +1837,13 @@ static void asm_cnew(ASMState *as, IRIns *ir)
> > > >  
> > > >    /* Increment cdatanum counter by address directly. */
> > > >    emit_i8(as, 1);
> > > > +#if LJ_GC64
> > > > +  emit_rmro(as, XO_ARITHi8, XOg_ADD|REX_64, RID_DISPATCH,
> > > > +	    dispofs(as, &J2G(as->J)->gc.cdatanum));
> > > 
> > > Should we cast the disp to 32bit? Here
> > 
> > IIRC, in function calls, the arguments are converted to the types of the
> > corresponding parameters. <ofs> parameter in <emit_rmro> is int32_t
> > type, so I guess an explicit cast is not obligatory here, isn't it?
> > 
> > > https://wiki.osdev.org/X86-64_Instruction_Encoding#Displacement
> > > I see only a disp32. 
> > 
> > However, as you've already mentioned offline the *valid* dispofs values
> > fit 32-bit integers since the size of GG_State equals to 6344 bytes.
> 
> I thought it should cover the difference from assembly placement to the
> _G, but now I got it should cover only the one from dispatch table to 
> your gc stuff. 

Precisely.

> 
> > 
> > I surmise, these explicit casts around relate to the old dark times when
> > various compilers were not so good, so Mike had to add such casts
> > everywhere. I checked the machine code generated by GCC on my machine
> > and see no difference between two versions: with or without the cast.
> > 
> > > 
> > > > +#else
> > > >    emit_rmro(as, XO_ARITHi8, XOg_ADD, RID_NONE,
> > > >  	    ptr2addr(&J2G(as->J)->gc.cdatanum));
> > > > +#endif
> > > >    /* Combine initialization of marked, gct and ctypeid. */
> > > >    emit_movtomro(as, RID_ECX, RID_RET, offsetof(GCcdata, marked));
> > > >    emit_gri(as, XG_ARITHi(XOg_OR), RID_ECX,
> > > > -- 
> > > > 2.25.0
> > > > 
> > 
> > -- 
> > Best regards,
> > IM

-- 
Best regards,
IM

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

* Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86
  2020-10-14 13:53 [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86 Igor Munkin
  2020-10-14 18:18 ` Sergey Ostanevich
  2020-10-14 19:04 ` Sergey Kaplun
@ 2020-10-15  8:41 ` Kirill Yukhin
  2 siblings, 0 replies; 8+ messages in thread
From: Kirill Yukhin @ 2020-10-15  8:41 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

Hello,

On 14 окт 16:53, Igor Munkin wrote:
> This patch fixes the regression introduced in scope of
> 5f6775ae0e141422193ad9b492806834064027ca ('core: introduce various
> platform metrics'). As a result of the patch <cdatanum> displacement is
> misencoded when GC64 mode is enabled.
> 
> In X86 long mode 32-bit displacement is encoded either via SIB byte or
> is addressed relatively to RIP register value. The first approach is
> used in JIT for 32-bit addresses (i.e. when GC64 mode is disabled), but
> doesn't work for 64-bit ones. As a result all addresses to GG_State
> contents to be "hardcoded" on the trace are encoded relatively to
> RID_DISPATCH register (i.e. callee-safe R14 register) containing global
> dispatch table. For this purpose this register is not used by the JIT
> register allocator in GC64 build and not spoiled throughout LuaJIT VM
> cycle (and therefore trace execution).
> 
> NB: Since R14 is the additional GRP, the <add> instruction ought to be
> REX-prefixed.
> 
> Follows up tarantool/tarantool#5187
> 
> Reported-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> Signed-off-by: Igor Munkin <imun@tarantool.org>

I've checked your patch into 1.10, 2.4, 2.5 and master.

--
Regards, Kirill Yukhin

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

end of thread, other threads:[~2020-10-15  8:41 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 13:53 [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86 Igor Munkin
2020-10-14 18:18 ` Sergey Ostanevich
2020-10-14 18:31   ` Igor Munkin
2020-10-14 20:11     ` Sergey Ostanevich
2020-10-14 20:13       ` Igor Munkin
2020-10-14 19:04 ` Sergey Kaplun
2020-10-14 19:22   ` Igor Munkin
2020-10-15  8:41 ` Kirill Yukhin

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