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 AD9056EC40; Wed, 2 Jun 2021 15:04:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AD9056EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1622635468; bh=eu17fHXsSJZwduMkiDSa/+sPgKxHcdY/BHqA1KL97to=; h=In-Reply-To:Date:References:To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=AEdaOwAzssElrLm2CNCNM0/as1Yyz8k0gG7cOas6Cq9hsK5HoZkge5w/QHwRZ0pvB 9gIROHqn67Ngy/Pkfljd4+JalvAazufDsh7ZfXbH1oA0yn2R8kU+4yjKbWb5rEr3fM TwyHW9wBwbIdBCZsGTiQYu2cTn3qWuuhJGiqttTQ= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 647ED6EC40 for ; Wed, 2 Jun 2021 15:04:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 647ED6EC40 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1loPbm-0002yG-6B; Wed, 02 Jun 2021 15:04:26 +0300 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) In-Reply-To: <51e2abaf644791331a077cab0852aa54d04941ff.1621859367.git.skaplun@tarantool.org> Date: Wed, 2 Jun 2021 15:04:25 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <225B5D75-E1D0-4137-8E78-FC78EE6952A0@tarantool.org> References: <51e2abaf644791331a077cab0852aa54d04941ff.1621859367.git.skaplun@tarantool.org> To: Sergey Kaplun X-Mailer: Apple Mail (2.3654.60.0.2.21) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9D5B0DA836B685C544BBC2A69B1B4100B389BF69B7A224D7C182A05F5380850402994565CABB41566244AC62A372B328B5D111DCD6090AA45AEFD3376D4205AA2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7BF6702EC5472AA0FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006375080A183704825A98638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D87E1A68BB36CA47509DDADD5FC626A1C3117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD1828451B159A507268D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6C94F115956DE4A7A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5C0D20240100CE6FCCCC5BDC5FAA550FC3170350645766633D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344B4608D01B1CB59AB94CE5DF7A5547BC33F269366CCA23DB577B5EC797C5361E9AB469BE6BB060DC1D7E09C32AA3244C3C8A45EBCA1C095BF0E428E31DA55ED9795D98D676DD64D0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj+mfSpkNmA2ro5T7pWSYheQ== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822687666D1A4D433ECC195B933D5F74FC9D86460CE94ACC410B98B9A657DE05FA7FB559BB5D741EB96F714CF339B3F4B0DA5517EFC3E06706F67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/4] ARM, ARM64, PPC: Fix TSETR fallback. 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 Ostanevich via Tarantool-patches Reply-To: Sergey Ostanevich Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See my 3 cents below. Sergos > On 24 May 2021, at 16:27, Sergey Kaplun wrote: >=20 > From: Mike Pall >=20 > Thanks to Javier Guerra Giraldez. >=20 > (cherry picked from commit ae20998ff5aaacc8e3afd46c64e28a8e039b58a1) >=20 > This patch fixes the issue introduced by commits > f307d0adafc7e35d2dc1c461d50f6572c5e6bca8 ('ARM64: Add build > infrastructure and initial port of interpreter.') for arm64 and > 73ef845fcaf65937ad63e9cf6b681cb3e61f4504 ('Add special bytecodes for > builtins.') for arm and ppc. Within the mentioned commits the new > bytecode TSETR is introduced for the corresponding architectures. >=20 > When the new index of the table processed during this bytecode is the > integer, that is greater than asize of the table, the VM fallbacks to > vmeta_tsetr, for calling > lj_tab_setinth(lua_State *L, GCtab *t, int32_t key). The first = argument > CARG1 is not set by the VM and contains an invalid value, so the > mentioned call leads to crash. > This patch adds the missed set of CARG1 to the right value. >=20 > Sergey Kaplun: > * added the description and the test for the problem >=20 > Resolves tarantool/tarantool#6084 > Part of tarantool/tarantool#5629 > --- > src/vm_arm.dasc | 1 + > src/vm_arm64.dasc | 1 + > src/vm_ppc.dasc | 1 + > test/tarantool-tests/CMakeLists.txt | 9 ++++--- > ...-missed-carg1-in-bctsetr-fallback.test.lua | 25 +++++++++++++++++++ > test/tarantool-tests/utils.lua | 22 ++++++++++++++++ > 6 files changed, 55 insertions(+), 4 deletions(-) > create mode 100644 = test/tarantool-tests/gh-6084-missed-carg1-in-bctsetr-fallback.test.lua >=20 > diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc > index ae2efdfd..21f7fecb 100644 > --- a/src/vm_arm.dasc > +++ b/src/vm_arm.dasc > @@ -701,6 +701,7 @@ static void build_subroutines(BuildCtx *ctx) > |->vmeta_tsetr: > | str BASE, L->base > | .IOS mov RC, BASE > + | mov CARG1, L > | str PC, SAVE_PC > | bl extern lj_tab_setinth // (lua_State *L, GCtab *t, int32_t = key) > | // Returns TValue *. > diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc > index f783428f..6bf59509 100644 > --- a/src/vm_arm64.dasc > +++ b/src/vm_arm64.dasc > @@ -711,6 +711,7 @@ static void build_subroutines(BuildCtx *ctx) > |->vmeta_tsetr: > | sxtw CARG3, TMP1w > | str BASE, L->base > + | mov CARG1, L > | str PC, SAVE_PC > | bl extern lj_tab_setinth // (lua_State *L, GCtab *t, int32_t = key) > | // Returns TValue *. > diff --git a/src/vm_ppc.dasc b/src/vm_ppc.dasc > index 62e9b681..3f48b7ff 100644 > --- a/src/vm_ppc.dasc > +++ b/src/vm_ppc.dasc > @@ -995,6 +995,7 @@ static void build_subroutines(BuildCtx *ctx) > | > |->vmeta_tsetr: > | stp BASE, L->base > + | mr CARG1, L > | stw PC, SAVE_PC > | bl extern lj_tab_setinth // (lua_State *L, GCtab *t, int32_t = key) > | // Returns TValue *. > diff --git a/test/tarantool-tests/CMakeLists.txt = b/test/tarantool-tests/CMakeLists.txt > index 475e2e5d..2fdb4d1f 100644 > --- a/test/tarantool-tests/CMakeLists.txt > +++ b/test/tarantool-tests/CMakeLists.txt > @@ -61,11 +61,12 @@ add_subdirectory(lj-flush-on-trace) > add_subdirectory(misclib-getmetrics-capi) >=20 > # The part of the memory profiler toolchain is located in tools > -# directory and auxiliary tests-related modules are located in the > -# current directory (but tests are run in the binary directory), > -# so LUA_PATH need to be updated. > +# directory, jit, profiler, and bytecode toolchains are located > +# in src/ directory and auxiliary tests-related modules are > +# located in the current directory (but tests are run in the > +# binary directory), so LUA_PATH need to be updated. > set(LUA_PATH > - = "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${PROJECT_SOURCE_DIR}/tools/?.lua" > + = "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${PROJECT_SOURCE_DIR}/tools/?.lua\;${P= ROJECT_SOURCE_DIR}/src/?.lua" > ) > set(LUA_TEST_SUFFIX .test.lua) > set(LUA_TEST_FLAGS --failures --shuffle) > diff --git = a/test/tarantool-tests/gh-6084-missed-carg1-in-bctsetr-fallback.test.lua = b/test/tarantool-tests/gh-6084-missed-carg1-in-bctsetr-fallback.test.lua > new file mode 100644 > index 00000000..26344274 > --- /dev/null > +++ = b/test/tarantool-tests/gh-6084-missed-carg1-in-bctsetr-fallback.test.lua > @@ -0,0 +1,25 @@ > +local tap =3D require("tap") > +local utils =3D require("utils") Sorry, but s-ostanevich:tarantool-tests s.ostanevich$ egrep -l "\.*\"" = *.lua | wc -l 6 s-ostanevich:tarantool-tests s.ostanevich$ egrep -l "\.*\'" = *.lua | wc -l 14 clearly votes for require(=E2=80=98tap') against require("tap=E2=80=9D) > + > +local test =3D tap.test("gh-6084-missed-carg1-in-bctsetr-fallback") > +test:plan(1) > + > +-- Bytecode TSETR appears only in built-ins libraries, when doing > +-- fixups for fast function written in Lua (i.e. `table.move()`), > +-- by replacing all TSETV bytecodes with the TSETR. > +-- See for more details. > + > +-- This test checks that fallback path, when the index of the new > +-- set element is greater than the table's asize, doesn't lead > +-- to a crash. > + > +-- We need to make sure the bytecode is present in the chosen > +-- built-in to make sure our test is still valid. > +assert(utils.hasbc(table.move, "TSETR")) > + > +-- Empty table has asize equals 0. Just copy its element (equals > +-- nil) to the field by index 1 > 0, to fallback inside TSETR. > +table.move({}, 1, 1, 1) I would like to see the move is correctly performed, rather the fact there were no crash. It gives a bigger space for unexpected behavior. > + > +test:ok(true) > +os.exit(test:check() and 0 or 1) > diff --git a/test/tarantool-tests/utils.lua = b/test/tarantool-tests/utils.lua > index c0403cf1..61d4de7a 100644 > --- a/test/tarantool-tests/utils.lua > +++ b/test/tarantool-tests/utils.lua > @@ -2,11 +2,14 @@ local M =3D {} >=20 > local ffi =3D require('ffi') > local tap =3D require('tap') > +local bc =3D require('jit.bc') >=20 > ffi.cdef([[ > int setenv(const char *name, const char *value, int overwrite); > ]]) >=20 > +local function noop() end Name of this one in a patch that messess with bytecodes is confusing. = Could it be a simpler one, like =E2=80=98empty=E2=80=99? > + > local function luacmd(args) > -- arg[-1] is guaranteed to be not nil. > local idx =3D -2 > @@ -89,4 +92,23 @@ function M.tweakenv(condition, variable) > ffi.C.setenv(variable, testvar, 0) > end >=20 > +function M.hasbc(f, bytecode) > + assert(type(f) =3D=3D 'function', 'argument #1 should be a = function') > + assert(type(bytecode) =3D=3D 'string', 'argument #2 should be a = string') > + local hasbc =3D false > + -- Check the bytecode entry line by line. > + local out =3D { > + write =3D function(out, line) > + if line:match(bytecode) then > + hasbc =3D true > + out.write =3D noop > + end > + end, > + flush =3D noop, > + close =3D noop, > + } > + bc.dump(f, out) > + return hasbc > +end > + > return M > --=20 > 2.31.0 >=20