From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 EBEA1469719 for ; Wed, 14 Oct 2020 23:24:26 +0300 (MSK) Date: Wed, 14 Oct 2020 23:13:49 +0300 From: Igor Munkin Message-ID: <20201014201349.GL18920@tarantool.org> References: <20201014181804.GD2885@tarantool.org> <20201014183136.GF18920@tarantool.org> <20201014201132.GE2885@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201014201132.GE2885@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, On 14.10.20, Sergey Ostanevich wrote: > > > Thanks! > > LGTM. Added your tag: | Reviewed-by: Sergey Ostanevich > > > 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 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 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