From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id EB04CE7FADF; Fri, 13 Dec 2024 15:54:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EB04CE7FADF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1734094444; bh=tOq1asIcRWN8f7gbF9sNYoF8V5LgjowWyC/ygh7CEKc=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=iBIG89N9bSvL1KzQNG0I1q9Td8GN3HzarBLyxH+js9HBBcELkZNvWppp13OIIJIRz mKkcDsMBxeqLX3K8vTyAMNaBS91Fv0DaztIM0GhmUHaKAacBJx9AmDIAEZiyF9es6v ApCtVY/KyoTYp84C97emUD6UvAcNcQZTAUlFdfrA= Received: from send128.i.mail.ru (send128.i.mail.ru [89.221.237.223]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C412A485A21 for ; Fri, 13 Dec 2024 15:54:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C412A485A21 Received: by exim-smtp-bf6fb4755-7r7zg with esmtpa (envelope-from ) id 1tM5B9-000000005UU-26Oa; Fri, 13 Dec 2024 15:54:00 +0300 Content-Type: multipart/alternative; boundary="------------Q7w1x8ae28z0zHI6vAdv2lZg" Message-ID: Date: Fri, 13 Dec 2024 15:54:00 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Maksim Tiushev Cc: tarantool-patches@dev.tarantool.org References: In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD911C40A3FA7A6588357826656CB4EFC778D9CA92F05AA0A92182A05F53808504044D6161638054A463DE06ABAFEAF6705523C171277C0D139E01BDA35A9C8DF0ACE3F7EDB596DBB80 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71C678EF579ACBE18EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637991D0D3E51C637188F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB553375669B93FEA0AB268975DDBB5A7469346B39FB934A80C6767A580C2C093071F64680389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0ECC8AC47CD0EDEFF8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B636DA1BED736F9328CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C22498BFD6B1B042489AC3AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F79006372D13D82DB4E1BCE9EC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5278DA827A17800CE76631511D42670FFE2EB15956EA79C166176DF2183F8FC7C0E4A630A5B664A4FF725E5C173C3A84C3DB8B71E42BA00C4F35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A572CC8CCD74B0CC805002B1117B3ED696EF500C68C65C0A62250A03108B67251B823CB91A9FED034534781492E4B8EEADAE4FDBF11360AC9BBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF7E7FC4A82ECE6B00696826301DAE2C2EF59965E6ECF6E16DAB2545E90DA48AADA161E968D9DBB3498E708AA2021A9CC252F4EF0BA96696D2BC626C8B44F3D2A11ACFD9216F7433305F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojgFHDrhqZ0P9r0bQBxLbZ7Q== X-Mailru-Sender: 520A125C2F17F0B1E52FEF5D219D6140E8C403C545936EA7D27678DDAA806314DF707C26997DF8F10152A3D17938EB451EB5A0BCEC6A560B3DDE9B364B0DF289BE2DA36745F2EEB5CEBA01FB949A1F1EEAB4BC95F72C04283CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 luajit 1/3] Ensure full init of IR_NOP instructions. X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------Q7w1x8ae28z0zHI6vAdv2lZg Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hello, Sergey, thanks for the patch! LGTM with a minor comment. On 11.12.2024 16:21, Sergey Kaplun wrote: > From: Mike Pall > > (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; --------------Q7w1x8ae28z0zHI6vAdv2lZg Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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;
--------------Q7w1x8ae28z0zHI6vAdv2lZg--