Re: [Tarantool-patches] [PATCH luajit 03/19] MIPS: Fix handling of spare long-range jump slots.

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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
>