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 D6C2D4B9FAF; Wed, 21 Jun 2023 12:04:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D6C2D4B9FAF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1687338285; bh=4dJ2cqvWIzVhwlR423nDd8FsNIdtsxmcjWNXCrB76N8=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=H39bfBPeOvWFOGdCEXxi7eoZziVGUz6LveNyA9DqKQpYy9KYt5ahSrocJz9UQ+7pJ jJQ7TsR+5cC0jsU5Pl45eVOk0d7Dnk9SImIkYzbR/S26SPCju9XhEUH7gMBrxpspBT dnvc5oldVYJVVzIPPP9V8OtuC6o4d6KzERtajHNw= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [95.163.41.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 947CA4B9FAF for ; Wed, 21 Jun 2023 12:04:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 947CA4B9FAF Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1qBtla-008USh-Ma; Wed, 21 Jun 2023 12:04:43 +0300 Date: Wed, 21 Jun 2023 12:00:28 +0300 To: Maxim Kokryashkin Message-ID: References: <1686907410.965498797@f534.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1686907410.965498797@f534.i.mail.ru> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD95D99986233CC4DDCD825FF3AA070ECA3C2EE242F31DB0C21182A05F538085040678D955D6B3292B5C94C0778BB237986F8E34AA04B9DA6111D967CDF05098C5F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE771540F9ECFC94C4BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063727BBC20C3D5F36038638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85D0549E954509F71C60EA749AC9AF3B1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735200AC5B80A05675ACD6FD1C55BDD38FC3FD2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEFAD5A440E159F97D96C9B5BF839F39F6D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3F8BD4E506CFA3D889735652A29929C6CC4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377F02F59195295693EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A552498D8D0A1A90DE5C7014727454DF2CB77B9BD8B52F3072F87CCE6106E1FC07E67D4AC08A07B9B0CE135D2742255B35CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34D2AC226BFD455724F558F24412C89DAAF935004E4266E165ABFE2E14CD3EA92EA1B6931A107BC6E11D7E09C32AA3244CE1F179134890D0B08E8879CC39A92E9CE3D93501275E802F85A42E4C463514DC5DA084F8E80FEBD3202CD0F03380D9577A83BD0C44CE203720ABEDE4BBDD9CDD X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojw7uTMtz3/lzeFX+z693qSQ== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A76921F1BAE48541F811C94C0778BB2379867345D355D45E21A6DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] Fix use-def analysis for vararg functions. 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Maxim! Thanks for the review! Fixed your comments on the branch! On 16.06.23, Maxim Kokryashkin wrote: > > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. >   > >  > >>From: Mike Pall > >> > >>Reported by Shmuel Zeigerman. > >> > >>(cherry-picked from commit 0e53a314d7910898e1ea5ba90385d43e8a6c5e57) > >> > >>Use-def analysis for BC_FUNCV may consider slots greater than amount of > >Typo: s/than/than the/ Fixed. > >>non-vararg parameters as dead slots due to early return for BC_RET > >Typo: s/due to/due to the/ Fixed. > >Also, it’d be nice to mention the exact spot where the early return > >is use-def analysis happens. Added. > >>emitted before usage of BC_VARG. This patch restricts the maxslot to be > >>analyzed in such case with amount of parameters for the prototype of the > >Typo: s/with/with the/ Fixed. > >>current function being recorded. > >> > >>Sergey Kaplun: > >>* added the description and the test for the problem > >> > >>Part of tarantool/tarantool#8516 > >>Relates to tarantool/tarantool#8718 > >>--- > >> src/lj_snap.c | 6 +++-- > >> .../lj-704-bc-varg-use-def.test.lua | 27 ++++++++++++++++++- > >> 2 files changed, 30 insertions(+), 3 deletions(-) > >> > >>diff --git a/src/lj_snap.c b/src/lj_snap.c > >>index 5bbe8498..a063c316 100644 > >>--- a/src/lj_snap.c > >>+++ b/src/lj_snap.c > >>@@ -301,8 +301,10 @@ static BCReg snap_usedef(jit_State *J, uint8_t *udf, > >> void lj_snap_purge(jit_State *J) > >> { > >>   uint8_t udf[SNAP_USEDEF_SLOTS]; > >>- BCReg maxslot = J->maxslot; > >>- BCReg s = snap_usedef(J, udf, J->pc, maxslot); > >>+ BCReg s, maxslot = J->maxslot; > >>+ if (bc_op(*J->pc) == BC_FUNCV && maxslot > J->pt->numparams) > >>+ maxslot = J->pt->numparams; > >>+ s = snap_usedef(J, udf, J->pc, maxslot); > >>   for (; s < maxslot; s++) > >>     if (udf[s] != 0) > >>       J->base[s] = 0; /* Purge dead slots. */ > >>diff --git a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua > >>index c3ba65dd..38606686 100644 > >>--- a/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua > >>+++ b/test/tarantool-tests/lj-704-bc-varg-use-def.test.lua > >>@@ -6,7 +6,7 @@ local test = tap.test('lj-704-bc-varg-use-def'):skipcond({ > >>   ['Test requires JIT enabled'] = not jit.status(), > >> }) > >>  > >>-test:plan(1) > >>+test:plan(2) > >>  > >> -- XXX: we don't really need to store this builtins, but this is > >> -- reduces `jitdump()` output for reader significantly. > >>@@ -62,4 +62,29 @@ wrap(ON_TRACE_VALUE) > >>  > >> test:ok(result ~= 0, 'use-def analysis for BC_VARG') > >>  > >>+-- Now check the same case, but with BC_RET before the BC_VARG, > >>+-- so use-def analysis will take early return case. > >Same as in the commit message, please mention the exact spot. > >>+-- See `snap_usedef()` in for details. > >>+-- The test checks that slots greater than `numparams` are not > >>+-- purged. > >>+local function varg_ret_bc(...) > >>+ -- XXX: This branch contains BC_RET. See the comment above. > >>+ -- luacheck: ignore > >>+ if false then else end > >>+ local slot = ({...})[1] > >>+ return fmod(ARG_ON_RECORDING, slot) > >Is there any reason why it has to be `fmod`? Added the comment. > >>+end > >>+ > >>+local function wrap_ret_bc(arg) > >>+ _, result = pcall(varg_ret_bc, arg) > >>+end > >Is it possible to adapt the `wrap` function from the test > >for the previous patch, so it can be used for both tests? Unfortunately no, because we want to record not wrap function (at the second test), but the one is pcall-ed. Added the comment. > >>+ > >>+-- Record trace with the 0 result. > >>+wrap_ret_bc(ARG_ON_RECORDING) > >>+wrap_ret_bc(ARG_ON_RECORDING) > >>+-- Record trace with the non-zero result. > >>+wrap_ret_bc(ON_TRACE_VALUE) > >>+ > >>+test:ok(result ~= 0, 'use-def analysis for FUNCV with return before BC_VARG') > >>+ > >> os.exit(test:check() and 0 or 1) > >>-- > >>2.34.1 > >-- > >Best regards, > >Maxim Kokryashkin > >  -- Best regards, Sergey Kaplun