Re: [Tarantool-patches] [PATCH luajit 1/2] Fix register mask for stack check in head of side trace.

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

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few nits below.
On Wed, Oct 11, 2023 at 06:04:09PM +0300, Sergey Kaplun wrote:
> From: Mike Pall <mike>
> 
> Analyzed by Sergey Kaplun.
> 
> (cherry-picked from commit b7a8c7c184257858699454408420dd5f0b6c8a75)
> 
> Assume we have parent and child traces with the following IRs from the
> IR dump:
> 
> Parent:
> | 0009 rax   >  tab TNEW   0     0
> | 0010          p32 FLOAD  0008  tab.node
> | 0011          p32 HREFK  0010  "Name" @1
> | 0012  {0008}  tab HSTORE 0011  0009
> | ....              SNAP   2    [ ---- 0001 0002 0008 ---- ]
> | 0013  {sink}  tab TNEW   0     0
> | 0014  {0008}  fal HSTORE 0011  false
> | ....              SNAP   3    [ ---- 0001 0002 0008 ---- ]
> 
> Child:
> | 0001 r15      tab SLOAD  1     PI
> | 0002 rbp      tab SLOAD  2     PI
> | 0003          tab PVAL   9
> 
> As we can see from the trace dump above, the `rax` register is missing
> in the `0003 PVAL` IR for the side trace -- so it is assumed to be
> available in the allow RegSet inside `asm_stack_check()` and its value
> is spoiled during this check, so if we are restoring from the 3rd
Typo: s/spoiled/spilled/
> snapshot by stack overflow -- we are in trouble.
> 
> The moment when IR is spoiled is when we set a hint on the register
Typo: s/spoiled/spilled/
> inherited from the parent trace (see `asm_setup_regsp()` for details).
> The 0th register (`rax`) shapeshifts into `RID_NONE`. Hence, when
> collecting register dependencies from the parent trace, `0003 PVAL` is
> considered the IR with `RID_NONE`, i.e., without an assigned register.
> So, this register is considered free (picked as bottom from the free
> set) in the `asm_stack_check()` and is used for stack overflow check, so
> the table reference is gone.
> 
> This patch introduces another register set for the context of the parent
> trace to use in the stack check. All registers used on the child trace
> are excluded from this set.
> 
> The test case for this patch is omitted since it requires specific
> register allocation, which is hard to construct and not stable in any
> future patches.
> 
> Part of tarantool/tarantool#9145
> 
> Sergey Kaplun:
> * added the description for the problem
> ---
>  src/lj_asm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index 3a1909d5..ca06860a 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c
> @@ -1859,6 +1859,7 @@ static void asm_head_side(ASMState *as)
>    IRRef1 sloadins[RID_MAX];
>    RegSet allow = RSET_ALL;  /* Inverse of all coalesced registers. */
>    RegSet live = RSET_EMPTY;  /* Live parent registers. */
> +  RegSet pallow = RSET_GPR;  /* Registers needed by the parent stack check. */
>    IRIns *irp = &as->parent->ir[REF_BASE];  /* Parent base. */
>    int32_t spadj, spdelta;
>    int pass2 = 0;
> @@ -1899,6 +1900,7 @@ static void asm_head_side(ASMState *as)
>        sloadins[rs] = (IRRef1)i;
>        rset_set(live, rs);  /* Block live parent register. */
>      }
> +    if (!ra_hasspill(regsp_spill(rs))) rset_clear(pallow, regsp_reg(rs));
>    }
>  
>    /* Calculate stack frame adjustment. */
> @@ -2015,7 +2017,7 @@ static void asm_head_side(ASMState *as)
>      ExitNo exitno = as->J->exitno;
>  #endif
>      as->T->topslot = (uint8_t)as->topslot;  /* Remember for child traces. */
> -    asm_stack_check(as, as->topslot, irp, allow & RSET_GPR, exitno);
> +    asm_stack_check(as, as->topslot, irp, pallow, exitno);
>    }
>  }
>  
> -- 
> 2.42.0
>