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 9F1736ECE3; Fri, 15 Jul 2022 17:46:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9F1736ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1657896388; bh=zj9DXFVRnp3kYCvTgiSsGouK7zad8fhfWsc2CoqrLMI=; 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=VcP4stvyJvRFaZPTT2hEfDK9yBSI4Su9etxdtJ9YjbQIMoDNik7pGrmHocK+93L8D +LuM2Ou/nYCl+Uq0prQGL+HY10rgY8sNZ7fVhxasmo5MTZBowmV7nynW64FTbEo8yi bP0D9cbqcOc+xHTueAs02l3UzShYKj6OLrZUsEVk= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (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 620CC6ECE3 for ; Fri, 15 Jul 2022 17:46:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 620CC6ECE3 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1oCMaI-0006TV-A6; Fri, 15 Jul 2022 17:46:26 +0300 Date: Fri, 15 Jul 2022 17:44:04 +0300 To: sergos Message-ID: References: <20220713095349.31718-1-skaplun@tarantool.org> <8B86F311-0882-4D94-9FD4-EB93EB11393C@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8B86F311-0882-4D94-9FD4-EB93EB11393C@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD97CF746166DCF1A14604890031D0BC59A233FD1C432F03131182A05F5380850401D4FF77DA532BFAF3067B7D3907B1E35AE1D8D8FDFEC4E9981D1D105C9C91EEC X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75644E22E05AA81AEB287FD4696A6DC2FA8DF7F3B2552694A4E2F5AFA99E116B42401471946AA11AF0E30A4C9C8E338DA4B5FAD37B56A37BD8F08D7030A58E5AD1A62830130A00468AEEEE3FBA3A834EE7353EFBB55337566D71D610BA8056A10741543DA0FF033826F93F4F9C70B55ABA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE75A31C54DA8CF07A79FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C390BCC82C2C62A6D1117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637BC468E7E89D8C5D6EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-8FC586DF: 6EFBBC1D9D64D975 X-C1DE0DAB: 9604B64F49C60606AD91A466A1DEF99B296C473AB1E142185AC9E3593CE4B31AB1881A6453793CE9274300E5CE05BD4401A9E91200F654B0D4C601CEA03E70EED71D610BA8056A10741543DA0FF033826F93F4F9C70B55AB9C2B6934AE262D3EE7EAB7254005DCED8DA55E71E02F9FC08E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B0359B88B232A0C6D560D64E7BB1539104ED113F63AD76AB3970B5B7EFAFEA41CAFBF756732A0D531D7E09C32AA3244CB98463EEA69520751EF83617DFF3EE05CE0B41342B755BCDFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojpf3/rFe+bF9obniFWQw0hQ== X-Mailru-Sender: 689FA8AB762F7393CC2E0F076E87284EA912EFE5F46C96E1AFE4FEC781DD56E50FBE9A32752B8C9C2AA642CC12EC09F1FB559BB5D741EB962F61BD320559CF1EFD657A8799238ED55FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] LJ_GC64: Fix IR_VARG offset for fixed number of results. 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, Sergos! Thanks for the review! On 14.07.22, sergos wrote: > Hi! > > Thanks for the patch, some updates to the test. > > Regards, > Sergos > > > On 13 Jul 2022, at 12:53, Sergey Kaplun wrote: > > > > From: Mike Pall > > > > Reported by George Vaintrub. Fixed by Sergey Kaplun. > > > > (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1) > > > > This bug occurs when recording `BC_VARG` with the following conditions: > > 1) varargs undefined on trace. > Later in the test you just mention its size should be bigger than results’ one? I meant that the __content__ of these slots is undefined, because the trace is started inside current function and we don't know the caller's context. So we can't just copy these slots. > > > 2) known fixed number of results. > > > > For this case the vararg slots loads via `IR_VLOAD` by offset from > are loaded by using an > > > vararg base. In GC64 mode this offset was miscounting due to missing > the the is miscounted > > > `LJ_FR2` correction in the base TRef calculation. As the result the > > wrong (+1) vararg slot is used. > > > > This patch adds the missing the aforementioned `LJ_FR2` correction. > xxx I've updated commit message to the following: Branch is force-pushed. =================================================================== LJ_GC64: Fix IR_VARG offset for fixed number of results. Reported by George Vaintrub. Fixed by Sergey Kaplun. (cherry picked from commit 6bda30d8c745b3963ba870221b9be6acdffed9b1) This bug occurs when recording `BC_VARG` with the following conditions: 1) A trace is started inside current vararg function, so the content of varargs slots is undefined for the trace. 2) Known fixed number of results. For this case the vararg slots are loaded by `IR_VLOAD` using an offset from the vararg base. In the GC64 mode this offset is miscounted due to missing `LJ_FR2` correction in the base TRef calculation. As the result the wrong (+1) vararg slot is used. This patch adds the missing aforementioned `LJ_FR2` correction. Sergey Kaplun: * added the description and the test for the problem Resolves tarantool/tarantool#7172 Part of tarantool/tarantool#7230 =================================================================== > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Resolves tarantool/tarantool#7172 > > Part of tarantool/tarantool#7230 > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-864-varg-rec-base-offset-full-ci > > Issues: > > * https://github.com/tarantool/tarantool/issues/7172 > > * https://github.com/LuaJIT/LuaJIT/issues/864 > > > > src/lj_record.c | 2 +- > > .../lj-864-varg-rec-base-offset.test.lua | 25 +++++++++++++++++++ > > 2 files changed, 26 insertions(+), 1 deletion(-) > > create mode 100644 test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua > > > > diff --git a/src/lj_record.c b/src/lj_record.c > > index a11f3712..9e2e1d9e 100644 > > --- a/src/lj_record.c > > +++ b/src/lj_record.c > > @@ -1794,7 +1794,7 @@ static void rec_varg(jit_State *J, BCReg dst, ptrdiff_t nresults) > > emitir(IRTGI(IR_EQ), fr, > > lj_ir_kint(J, (int32_t)frame_ftsz(J->L->base-1))); > > vbase = emitir(IRT(IR_SUB, IRT_IGC), REF_BASE, fr); > > - vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8)); > > + vbase = emitir(IRT(IR_ADD, IRT_PGC), vbase, lj_ir_kint(J, frofs-8*(1+LJ_FR2))); > Wherearemyspaces? > (nevermind, just a moan) Yes, but this style is preferred by Mike (see code below) :). > > > for (i = 0; i < nload; i++) { > > IRType t = itype2irt(&J->L->base[i-1-LJ_FR2-nvararg]); > > TRef aref = emitir(IRT(IR_AREF, IRT_PGC), > > diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua > > new file mode 100644 > > index 00000000..ca30f92f > > --- /dev/null > > +++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua > > @@ -0,0 +1,25 @@ > > +local tap = require('tap') > > + > > +-- Test file to demonstrate LuaJIT misbehaviour during recording > > +-- BC_VARG with nvarargs >= nresults in GC64 mode. > In the message you say it should be unknown. What’s the dirty truth is? > > > +-- See also https://github.com/LuaJIT/LuaJIT/issues/864, > > +-- https://github.com/tarantool/tarantool/issues/7172. > > +local test = tap.test('lj-864-varg-rec-base-offset') > > +test:plan(1) > > + > > +jit.opt.start('hotloop=1') > > + > > +local MAGIC = 42 > Should be enough to test against the first argument, no MAGIC :) > > > +local function test_rec_varg(...) > > + local slot1 > > + for _ = 1, 3 do > > + slot1 = ... > > + end > ++ args = {...} > +- return slot1 == args[1] > > +end I've rewritten test wo MAGIC :). =================================================================== diff --git a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua index ca30f92f..d74c3c2b 100644 --- a/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua +++ b/test/tarantool-tests/lj-864-varg-rec-base-offset.test.lua @@ -9,17 +9,17 @@ test:plan(1) jit.opt.start('hotloop=1') -local MAGIC = 42 local function test_rec_varg(...) - local slot1 + local trace_value, interp_value for _ = 1, 3 do - slot1 = ... + trace_value = ... end - return slot1 == MAGIC + interp_value = ... + return trace_value == interp_value end -- Test case for nvarargs >= nresults. Equality is not suitable -- due to failing assertion guard for type of loaded vararg slot. -test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording') +test:ok(test_rec_varg(42, 0), 'correct BC_VARG recording') os.exit(test:check() and 0 or 1) =================================================================== > > + > > +-- Test case for nvarargs >= nresults. Equality is not suitable > > +-- due to failing assertion guard for type of loaded vararg slot. > > +test:ok(test_rec_varg(MAGIC, 0), 'correct BC_VARG recording') > > + > > +os.exit(test:check() and 0 or 1) > > -- > > 2.34.1 > > > -- Best regards, Sergey Kaplun