From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id DAFA25C3F0F; Tue, 15 Aug 2023 14:13:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DAFA25C3F0F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692098020; bh=11V90LM/r7wfjuH+WwtxxarAp8cDWxKxgwaffVYhA5g=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=rDJ/glHAw0AwPFNjEaMeEPyg6wtP+BjLk8dXPV8mFHr/5eaygThnOXrLavK0WOy0g c7yX0fDwfl8V+556T+EB92YQNAOXETpyqwmN1auB9YTOuPpAvBtAA87ABvW/al1l7X fxH/pWRnPqhSGF6mGCpr3qy5PrhxC6UAWFt7Mfto= Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [95.163.41.66]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3724A5C3F0F for ; Tue, 15 Aug 2023 14:13:38 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3724A5C3F0F Received: by smtp43.i.mail.ru with esmtpa (envelope-from ) id 1qVrzV-00FNaR-1m; Tue, 15 Aug 2023 14:13:37 +0300 Date: Tue, 15 Aug 2023 14:13:37 +0300 To: Sergey Kaplun Message-ID: References: <7179245cf38c56a88bb8f3aa1bbeaf15402fcd1a.1691592488.git.skaplun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7179245cf38c56a88bb8f3aa1bbeaf15402fcd1a.1691592488.git.skaplun@tarantool.org> X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC8BEB87106826C459512B2000DD660D84D182A05F53808504052BB1376DAE390F6D0532EF1C6F82A20F727341896ED71768482BEF8D1D3F9DC X-C1DE0DAB: 0D63561A33F958A58897B72E3A33128802D8472448659321E69048E04F6EDF1BF87CCE6106E1FC07E67D4AC08A07B9B067F1C1C3ABB44F3ACB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527F25AF8D0F2EEA1275F9E24887CF477E375DBC4E065AC0F00DF2C698BB77E28BC438F074F5C2983D2337FD76D11AF80A0D28E2843392A4F0522878147484127B40EA455F16B58544A21C197AAF4D2E4732965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJ1ceUZTkowloJOrHaojo7g== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE407F9C72D0464D81F2F72881A37C64D5BA1AA34DFAC7EFFC4FCD51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! Thanks for the patch! LGTM, except for a few comments below. On Wed, Aug 09, 2023 at 06:35:52PM +0300, Sergey Kaplun via Tarantool-patches wrote: > From: Mike Pall > > Contributed by Djordje Kovacevic and Stefan Pejic. > > (cherry-picked from commit c7c3c4da432ddb543d4b0a9abbb245f11b26afd0) > > `asm_setup_jump()` in presumes that `sizeof(MCLink)` > is 8 bytes, but for MIPS64 its size is 16 bytes. This leads to incorrect Typo: s/to incorrect/to an incorrect/ > check in `asm_sparejump_setup()`, so mcode bottom is not updated. Typo: s/so mcode/so the mcode/ > > This patch fixes check of the MCLink offset from the mcbot. Typo: s/fixes check/fixes the check/ > Nevertheless, the emitting of spare jump slots is still incorrect, so > the introduced test still fails due to incorrect iteration through the Typo: s/due to/due to the/ > sparce table (the last slot is out of mcode range). > > This should be fixed via backporting of the commit > dbb78630169a8106b355a5be8af627e98c362f1e ("MIPS: Fix handling of > long-range spare jumps."). But it triggers the new unconditional > assert, that is added in this patch, mentioning that sizemcode is too > bit. So some workaround should be found, when this test will be enabled Typo: s/bit/big/ Typo: s/will be/is/ > for MIPS. > > Since test also validates the behaviour of long-range jumps to side > traces for arm64 and x64, and we have no testing for MIPS64 (yet), we > can leave it as is without a skipcond. > > Sergey Kaplun: > * added the description and the test for the problem > > Part of tarantool/tarantool#8825 > --- > src/lj_asm_mips.h | 9 +-- > src/lj_jit.h | 6 ++ > src/lj_mcode.c | 6 -- > ...x-mips64-spare-side-exit-patching.test.lua | 65 +++++++++++++++++++ > 4 files changed, 76 insertions(+), 10 deletions(-) > create mode 100644 test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > > diff --git a/src/lj_asm_mips.h b/src/lj_asm_mips.h > index 03215821..0e60fc07 100644 > --- a/src/lj_asm_mips.h > +++ b/src/lj_asm_mips.h > @@ -65,10 +65,9 @@ static Reg ra_alloc2(ASMState *as, IRIns *ir, RegSet allow) > static void asm_sparejump_setup(ASMState *as) > { > MCode *mxp = as->mcbot; > - /* Assumes sizeof(MCLink) == 8. */ > - if (((uintptr_t)mxp & (LJ_PAGESIZE-1)) == 8) { > + if (((uintptr_t)mxp & (LJ_PAGESIZE-1)) == sizeof(MCLink)) { > lua_assert(MIPSI_NOP == 0); > - memset(mxp+2, 0, MIPS_SPAREJUMP*8); > + memset(mxp, 0, MIPS_SPAREJUMP*2*sizeof(MCode)); > mxp += MIPS_SPAREJUMP*2; > lua_assert(mxp < as->mctop); > lj_mcode_sync(as->mcbot, mxp); > @@ -2486,7 +2485,9 @@ void lj_asm_patchexit(jit_State *J, GCtrace *T, ExitNo exitno, MCode *target) > if (!cstart) cstart = p-1; > } else { /* Branch out of range. Use spare jump slot in mcarea. */ > int i; > - for (i = 2; i < 2+MIPS_SPAREJUMP*2; i += 2) { > + for (i = (int)(sizeof(MCLink)/sizeof(MCode)); > + i < (int)(sizeof(MCLink)/sizeof(MCode)+MIPS_SPAREJUMP*2); > + i += 2) { > if (mcarea[i] == tjump) { > delta = mcarea+i - p; > goto patchbranch; > diff --git a/src/lj_jit.h b/src/lj_jit.h > index f2ad3c6e..cc8efd20 100644 > --- a/src/lj_jit.h > +++ b/src/lj_jit.h > @@ -158,6 +158,12 @@ typedef uint8_t MCode; > typedef uint32_t MCode; > #endif > > +/* Linked list of MCode areas. */ > +typedef struct MCLink { > + MCode *next; /* Next area. */ > + size_t size; /* Size of current area. */ > +} MCLink; > + > /* Stack snapshot header. */ > typedef struct SnapShot { > uint32_t mapofs; /* Offset into snapshot map. */ > diff --git a/src/lj_mcode.c b/src/lj_mcode.c > index 7184d3b4..c6361018 100644 > --- a/src/lj_mcode.c > +++ b/src/lj_mcode.c > @@ -272,12 +272,6 @@ static void *mcode_alloc(jit_State *J, size_t sz) > > /* -- MCode area management ----------------------------------------------- */ > > -/* Linked list of MCode areas. */ > -typedef struct MCLink { > - MCode *next; /* Next area. */ > - size_t size; /* Size of current area. */ > -} MCLink; > - > /* Allocate a new MCode area. */ > static void mcode_allocarea(jit_State *J) > { > diff --git a/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > new file mode 100644 > index 00000000..fdc826cb > --- /dev/null > +++ b/test/tarantool-tests/fix-mips64-spare-side-exit-patching.test.lua > @@ -0,0 +1,65 @@ > +local tap = require('tap') > +local test = tap.test('fix-mips64-spare-side-exit-patching'):skipcond({ > + ['Test requires JIT enabled'] = not jit.status(), > + ['Disabled on *BSD due to #4819'] = jit.os == 'BSD', > + -- Need to fix the MIPS behaviour first. Typo: s/Need to/We need to/ > + ['Disabled for MIPS architectures'] = jit.arch:match('mips'), > +}) > + > +local generators = require('utils').jit.generators > +local frontend = require('utils').frontend > + > +test:plan(1) > + > +-- Make compiler work hard. > +jit.opt.start( > + -- No optimizations at all to produce more mcode. > + 0, > + -- Try to compile all compiled paths as early as JIT can. > + 'hotloop=1', > + 'hotexit=1', > + -- Allow to use 2000 traces to avoid flushes. Typo: s/to use/compilation of up to/ > + 'maxtrace=2000', > + -- Allow to compile 8Mb of mcode to be sure the issue occurs. Typo: s/to compile/compilation of up to/ > + 'maxmcode=8192', > + -- Use big mcode area for traces to avoid using different Typo: s/using/usage of/ > + -- spare slots. > + 'sizemcode=256' > +) > + > +local MAX_SPARE_SLOT = 4 A link to the definition in `lj_asm_mips.h` would be nice to have. > +local function parent(marker) > + -- Use several side exit to fill spare exit space (default is Typo: s/side exit/side exits/ > + -- 4 slots, each slot has 2 instructions -- jump and nop). > + -- luacheck: ignore > + if marker > MAX_SPARE_SLOT then end > + if marker > 3 then end > + if marker > 2 then end > + if marker > 1 then end > + if marker > 0 then end > + -- XXX: use `fmod()` to avoid leaving the function and use > + -- stitching here. > + return math.fmod(1, 1) > +end > + > +-- Compile parent trace first. > +parent(0) > +parent(0) > + > +local parent_traceno = frontend.gettraceno(parent) > +local last_traceno = parent_traceno > + > +-- Now generate some mcode to forcify long jump with a spare slot. > +-- Each iteration provide different addresses and uses a different Typo: s/provide/provides/ > +-- spare slot. After it compile and execute new side trace. Typo: s/After it compile and execute/After that, compiles and executes a/ > +for i = 1, MAX_SPARE_SLOT + 1 do > + generators.fillmcode(last_traceno, 1024 * 1024) > + parent(i) > + parent(i) > + parent(i) > + last_traceno = misc.getmetrics().jit_trace_num > +end > + > +test:ok(true, 'all traces executed correctly') > + > +test:done(true) > -- > 2.41.0 >