[Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots.
Maxim Kokryashkin
m.kokryashkin at tarantool.org
Tue Aug 15 14:13:37 MSK 2023
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 <mike>
>
> Contributed by Djordje Kovacevic and Stefan Pejic.
>
> (cherry-picked from commit c7c3c4da432ddb543d4b0a9abbb245f11b26afd0)
>
> `asm_setup_jump()` in <src/lj_asm_mips.h> 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
>
More information about the Tarantool-patches
mailing list