From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D05B9469719 for ; Wed, 14 Oct 2020 21:42:14 +0300 (MSK) Date: Wed, 14 Oct 2020 21:31:36 +0300 From: Igor Munkin Message-ID: <20201014183136.GF18920@tarantool.org> References: <20201014181804.GD2885@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201014181804.GD2885@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] jit: fix cdatanum addressing for GC64 mode on x86 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org 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 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 instruction ought to be > > REX-prefixed. > > > > Follows up tarantool/tarantool#5187 > > > > Reported-by: Vladislav Shpilevoy > > Signed-off-by: Igor Munkin > > --- > > > > 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. parameter in 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