Hello, Sergey,
thanks for the patch! LGTM with a minor comment.
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;