[Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions.

[Sergey Bronnikov]

Hello, Sergey,

thanks for the patch! LGTM with a minor comment.

On 11.12.2024 16:21, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> (cherry picked from commit de6b1a11dd1a3349179084578c5d533be1c30234)
>
> Before this patch, Valgrind produces tons of warnings during the VMevent
> calls since the `IR_NOP` instruction isn't fully initialized. Hence, any
> parsing operations for it in handlers during `jit.dump()` leads to the
> "uninitialised value" error. This patch fixes the issue by the proper
> init of such IRs.

Please add to a comment that with these tests the problem could

  be reproduced:

test/tarantool-tests/lj-1128-table-new-opt-tnew.test.lua

test/tarantool-tests/fix-jit-dump-ir-conv.test.lua

test/tarantool-tests/unit-jit-parse.test.lua

>
> Sergey Kaplun:
> * added the description for the problem
>
> Needed for tarantool/tarantool#3705
> Part of tarantool/tarantool#10709
> ---
>   src/lj_asm.c     |  2 +-
>   src/lj_ir.h      |  8 ++++++++
>   src/lj_opt_dce.c |  5 +----
>   src/lj_opt_mem.c | 25 +++++--------------------
>   4 files changed, 15 insertions(+), 25 deletions(-)
>
> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index 5137d05c..f7540165 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c
> @@ -2377,7 +2377,7 @@ void lj_asm_trace(jit_State *J, GCtrace *T)
>     /* Ensure an initialized instruction beyond the last one for HIOP checks. */
>     /* This also allows one RENAME to be added without reallocating curfinal. */
>     as->orignins = lj_ir_nextins(J);
> -  J->cur.ir[as->orignins].o = IR_NOP;
> +  lj_ir_nop(&J->cur.ir[as->orignins]);
>   
>     /* Setup initial state. Copy some fields to reduce indirections. */
>     as->J = J;
> diff --git a/src/lj_ir.h b/src/lj_ir.h
> index 27c66f63..46e7413e 100644
> --- a/src/lj_ir.h
> +++ b/src/lj_ir.h
> @@ -587,4 +587,12 @@ static LJ_AINLINE int ir_sideeff(IRIns *ir)
>   
>   LJ_STATIC_ASSERT((int)IRT_GUARD == (int)IRM_W);
>   
> +/* Replace IR instruction with NOP. */
> +static LJ_AINLINE void lj_ir_nop(IRIns *ir)
> +{
> +  ir->ot = IRT(IR_NOP, IRT_NIL);
> +  ir->op1 = ir->op2 = 0;
> +  ir->prev = 0;
> +}
> +
>   #endif
> diff --git a/src/lj_opt_dce.c b/src/lj_opt_dce.c
> index 6948179c..41e36d99 100644
> --- a/src/lj_opt_dce.c
> +++ b/src/lj_opt_dce.c
> @@ -46,10 +46,7 @@ static void dce_propagate(jit_State *J)
>         irt_clearmark(ir->t);
>       } else if (!ir_sideeff(ir)) {
>         *pchain[ir->o] = ir->prev;  /* Reroute original instruction chain. */
> -      ir->t.irt = IRT_NIL;
> -      ir->o = IR_NOP;  /* Replace instruction with NOP. */
> -      ir->op1 = ir->op2 = 0;
> -      ir->prev = 0;
> +      lj_ir_nop(ir);
>         continue;
>       }
>       pchain[ir->o] = &ir->prev;
> diff --git a/src/lj_opt_mem.c b/src/lj_opt_mem.c
> index c9f1216c..a6e9a072 100644
> --- a/src/lj_opt_mem.c
> +++ b/src/lj_opt_mem.c
> @@ -375,10 +375,7 @@ TRef LJ_FASTCALL lj_opt_dse_ahstore(jit_State *J)
>   	    goto doemit;  /* No elimination possible. */
>   	/* Remove redundant store from chain and replace with NOP. */
>   	*refp = store->prev;
> -	store->o = IR_NOP;
> -	store->t.irt = IRT_NIL;
> -	store->op1 = store->op2 = 0;
> -	store->prev = 0;
> +	lj_ir_nop(store);
>   	/* Now emit the new store instead. */
>         }
>         goto doemit;
> @@ -479,10 +476,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J)
>   	    goto doemit;  /* No elimination possible. */
>   	/* Remove redundant store from chain and replace with NOP. */
>   	*refp = store->prev;
> -	store->o = IR_NOP;
> -	store->t.irt = IRT_NIL;
> -	store->op1 = store->op2 = 0;
> -	store->prev = 0;
> +	lj_ir_nop(store);
>   	if (ref+1 < J->cur.nins &&
>   	    store[1].o == IR_OBAR && store[1].op1 == xref) {
>   	  IRRef1 *bp = &J->chain[IR_OBAR];
> @@ -491,10 +485,7 @@ TRef LJ_FASTCALL lj_opt_dse_ustore(jit_State *J)
>   	    bp = &obar->prev;
>   	  /* Remove OBAR, too. */
>   	  *bp = obar->prev;
> -	  obar->o = IR_NOP;
> -	  obar->t.irt = IRT_NIL;
> -	  obar->op1 = obar->op2 = 0;
> -	  obar->prev = 0;
> +	  lj_ir_nop(obar);
>   	}
>   	/* Now emit the new store instead. */
>         }
> @@ -585,10 +576,7 @@ TRef LJ_FASTCALL lj_opt_dse_fstore(jit_State *J)
>   	    goto doemit;  /* No elimination possible. */
>   	/* Remove redundant store from chain and replace with NOP. */
>   	*refp = store->prev;
> -	store->o = IR_NOP;
> -	store->t.irt = IRT_NIL;
> -	store->op1 = store->op2 = 0;
> -	store->prev = 0;
> +	lj_ir_nop(store);
>   	/* Now emit the new store instead. */
>         }
>         goto doemit;
> @@ -839,10 +827,7 @@ TRef LJ_FASTCALL lj_opt_dse_xstore(jit_State *J)
>   	    goto doemit;  /* No elimination possible. */
>   	/* Remove redundant store from chain and replace with NOP. */
>   	*refp = store->prev;
> -	store->o = IR_NOP;
> -	store->t.irt = IRT_NIL;
> -	store->op1 = store->op2 = 0;
> -	store->prev = 0;
> +	lj_ir_nop(store);
>   	/* Now emit the new store instead. */
>         }
>         goto doemit;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20241213/522b3464/attachment.htm>