From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 DC768469719 for ; Wed, 14 Oct 2020 22:05:05 +0300 (MSK) Date: Wed, 14 Oct 2020 22:04:38 +0300 From: Sergey Kaplun Message-ID: <20201014190438.GA9302@root> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: Igor Munkin Cc: tarantool-patches@dev.tarantool.org 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 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. Nit: Also REX prefix is needed to specify 64-bit operand size, isn't it? > > 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)); > +#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