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 A3E3E6EC40; Fri, 4 Jun 2021 18:33:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A3E3E6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1622820811; bh=Q5GU7qmjA+3NjSOuqPmzynsCwh2qd/ABg6XdxFZ8TLg=; h=Date:In-Reply-To:To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Y+PHWuRRWNruGYeKQ5Zi+MOy3vauCiaIc/JJEsNpSt8z1QB6aHDY6sJqsqobC1GSQ 1BiAZp2PHymqFJ46YTN1wupYZ+MPTfVoFDLulWkgBp9LILY4ZSF4M+rvflR59O8Wlb X7q8CK9QpXVV9Q42IS6mRoVYRwJ5+gEJBimgov9I= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 1FEBB6EC40 for ; Fri, 4 Jun 2021 18:33:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1FEBB6EC40 Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1lpBpA-00026r-Oj; Fri, 04 Jun 2021 18:33:29 +0300 Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_14BC090F-1872-41B0-9BDA-F5D3B786CD76" Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Date: Fri, 4 Jun 2021 18:33:28 +0300 In-Reply-To: To: Sergey Kaplun References: <51e2abaf644791331a077cab0852aa54d04941ff.1621859367.git.skaplun@tarantool.org> <225B5D75-E1D0-4137-8E78-FC78EE6952A0@tarantool.org> X-Mailer: Apple Mail (2.3654.60.0.2.21) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D5B0DA836B685C549A9F97C297FFF2C725C7934AD8E7B4B9182A05F538085040DDD7FAF04DC315977C69821EAE69966B049208C3FF55F07AAA0EC1D356B06CC8 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AC4684DF4EC4B256EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063704C003DD579243BB8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8723DC553BD9645CB0016912E6FAA8CB1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BF1175FABE1C0F9B6A471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C837C4FEFBD186071C4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A59C52588F62CA6934675720C7355801724214FABE39D97148D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FBC5FED0552DA851410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346B222596F62B8FA9ACDA71087EDC6A7DE85ADF7A3D89768D39A844E59079B797A2B2AF60BF8AD0861D7E09C32AA3244C043E516206FAAC35B9F099D3CF153A131E098CBE561D6343FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojz99asgmzejqKyIiBcSZwog== X-Mailru-Sender: 3B9A0136629DC912F4AABCEFC589C81EBCEEE0084C455B9C8F933AE4C87478BFEF5024447D2A43FDAD07DD1419AC565FA614486B47F28B67C5E079CCF3B0523AED31B7EB2E253A9E112434F685709FCF0DA7A0AF5A3A8387 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" --Apple-Mail=_14BC090F-1872-41B0-9BDA-F5D3B786CD76 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thanks for the update, LGTM. As for LuaJIT code style - should it follow the Tarantool Lua one? https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guide/ = Sergos. > On 4 Jun 2021, at 16:12, Sergey Kaplun wrote: >=20 > Hi! >=20 > Thanks for the review! >=20 > On 02.06.21, Sergey Ostanevich wrote: >> Hi! >> Thanks for the patch! >>=20 >> See my 3 cents below. >>=20 >> Sergos >>=20 >>=20 >>> 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") >>=20 >> Sorry, but >>=20 >> s-ostanevich:tarantool-tests s.ostanevich$ egrep -l "\.*\"" = *.lua | wc -l >> 6 >> s-ostanevich:tarantool-tests s.ostanevich$ egrep -l "\.*\'" = *.lua | wc -l >> 14 >>=20 >> clearly votes for require(=E2=80=98tap') against require("tap=E2=80=9D)= >=20 > I've tried to follow the original code style from src/jit/ directory: >=20 > | src$ grep -l -P 'require.*"' */*.lua -r | wc -l > | 17 > | src$ grep -l -P "require.*'" */*.lua -r | wc -l > | 0 >=20 > Also, you count utils.lua 3 times. >=20 > But I don't mind :) > Branch is force-pushed. >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 > index 26344274..1a438c82 100644 > --- = 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 > @@ -1,7 +1,7 @@ > -local tap =3D require("tap") > -local utils =3D require("utils") > +local tap =3D require('tap') > +local utils =3D require('utils') >=20 > -local test =3D tap.test("gh-6084-missed-carg1-in-bctsetr-fallback") > +local test =3D tap.test('gh-6084-missed-carg1-in-bctsetr-fallback') > test:plan(1) >=20 > -- Bytecode TSETR appears only in built-ins libraries, when doing > @@ -15,7 +15,7 @@ test:plan(1) >=20 > -- 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")) > +assert(utils.hasbc(table.move, 'TSETR')) >=20 > -- Empty table has asize equals 0. Just copy its element (equals > -- nil) to the field by index 1 > 0, to fallback inside TSETR. >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > Side note: we should document our LuaJIT codestyle... >=20 >>=20 >>> + >>> +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) >>=20 >> 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. >=20 > Fixed. Branch is force-pushed. >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 > index 1a438c82..95bf3bd7 100644 > --- = 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 > @@ -2,7 +2,7 @@ local tap =3D require('tap') > local utils =3D require('utils') >=20 > local test =3D tap.test('gh-6084-missed-carg1-in-bctsetr-fallback') > -test:plan(1) > +test:plan(2) >=20 > -- Bytecode TSETR appears only in built-ins libraries, when doing > -- fixups for fast function written in Lua (i.e. `table.move()`), > @@ -17,9 +17,11 @@ test:plan(1) > -- built-in to make sure our test is still valid. > assert(utils.hasbc(table.move, 'TSETR')) >=20 > --- 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) > +-- `t` table has asize equals 1. Just copy its first element (1) > +-- to the field by index 2 > 1, to fallback inside TSETR. > +local t =3D {1} > +local res =3D table.move(t, 1, 1, 2) > +test:ok(t =3D=3D res, 'table.move returns the same table') > +test:ok(t[1] =3D=3D t[2], 'table.move is correct') >=20 > -test:ok(true) > os.exit(test:check() and 0 or 1) > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 >>=20 >>> + >>> +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 >>=20 >> 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? >=20 > Fixed. Branch is force-pushed. >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > diff --git a/test/tarantool-tests/utils.lua = b/test/tarantool-tests/utils.lua > index 61d4de7a..57932c5d 100644 > --- a/test/tarantool-tests/utils.lua > +++ b/test/tarantool-tests/utils.lua > @@ -8,7 +8,7 @@ ffi.cdef([[ > int setenv(const char *name, const char *value, int overwrite); > ]]) >=20 > -local function noop() end > +local function empty() end >=20 > local function luacmd(args) > -- arg[-1] is guaranteed to be not nil. > @@ -101,11 +101,11 @@ function M.hasbc(f, bytecode) > write =3D function(out, line) > if line:match(bytecode) then > hasbc =3D true > - out.write =3D noop > + out.write =3D empty > end > end, > - flush =3D noop, > - close =3D noop, > + flush =3D empty, > + close =3D empty, > } > bc.dump(f, out) > return hasbc > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 >>=20 >>> + >>> 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 >>=20 >=20 > --=20 > Best regards, > Sergey Kaplun --Apple-Mail=_14BC090F-1872-41B0-9BDA-F5D3B786CD76 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi!

Thanks = for the update, LGTM.

As for LuaJIT code style - should it follow the Tarantool Lua = one?
https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guid= e/

Sergos.

On 4 Jun 2021, at 16:12, Sergey = Kaplun <skaplun@tarantool.org> wrote:

Hi!

Thanks for the review!

On 02.06.21, Sergey Ostanevich wrote:
Hi!
Thanks for the patch!

See my 3 = cents below.

Sergos


On 24 May = 2021, at 16:27, Sergey Kaplun <skaplun@tarantool.org> wrote:

From: Mike Pall <mike>

Thanks to Javier Guerra Giraldez.

(cherry picked from commit = ae20998ff5aaacc8e3afd46c64e28a8e039b58a1)

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.

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.

Sergey Kaplun:
* added = the description and the test for the problem

Resolves tarantool/tarantool#6084
Part of = tarantool/tarantool#5629
---
src/vm_arm.dasc =             &n= bsp;           &nbs= p;     |  1 +
src/vm_arm64.dasc =             &n= bsp;           &nbs= p;   |  1 +
src/vm_ppc.dasc =             &n= bsp;           &nbs= p;     |  1 +
test/tarantool-tests/CMakeLists.txt =           |  9 = ++++---
...-missed-carg1-in-bctsetr-fallback.test.lua | 25 = +++++++++++++++++++
test/tarantool-tests/utils.lua =             &n= bsp;  | 22 ++++++++++++++++
6 files changed, 55 = insertions(+), 4 deletions(-)
create mode 100644 = test/tarantool-tests/gh-6084-missed-carg1-in-bctsetr-fallback.test.lua

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)

# 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/?.lu= a"
+ =  "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${PROJECT_SOURCE_DIR}/tools/?.lu= a\;${PROJECT_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.luanew 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 = "\<require\>.*\"" *.lua  | wc -l
      6
s-ostanevich:tarantool-tests s.ostanevich$ egrep -l = "\<require\>.*\'" *.lua  | wc -l
     14

clearly votes for require(=E2=80=98tap') against = require("tap=E2=80=9D)

I've tried to = follow the original code style from src/jit/ directory:

| src$ grep = -l -P 'require.*"' */*.lua -r | wc -l
| 17
| src$ grep -l -P "require.*'" */*.lua -r | wc -l
| 0

Also, you = count utils.lua 3 times.

But I don't mind :)
Branch is force-pushed.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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
index = 26344274..1a438c82 100644
--- = 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
@@ -1,7 +1,7 = @@
-local tap =3D = require("tap")
-local utils =3D require("utils")
+local tap =3D = require('tap')
+local utils =3D require('utils')

-local test =3D= tap.test("gh-6084-missed-carg1-in-bctsetr-fallback")
+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
@@ -15,7 = +15,7 @@ test:plan(1)

-- 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"))
+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.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Side note: we = should document our LuaJIT codestyle...


+
+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 <src/host/genlibbc.lua> 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.

Fixed. Branch is force-pushed.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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
index = 1a438c82..95bf3bd7 100644
--- = 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
@@ -2,7 +2,7 = @@ local tap =3D require('tap')
local utils =3D require('utils')

local test =3D = tap.test('gh-6084-missed-carg1-in-bctsetr-fallback')
-test:plan(1)
+test:plan(2)

-- Bytecode TSETR appears only in built-ins libraries, when = doing
-- fixups for = fast function written in Lua (i.e. `table.move()`),
@@ -17,9 = +17,11 @@ test:plan(1)
-- 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)
+-- `t` table = has asize equals 1. Just copy its first element (1)
+-- to the = field by index 2 > 1, to fallback inside TSETR.
+local t =3D = {1}
+local res =3D = table.move(t, 1, 1, 2)
+test:ok(t =3D=3D res, 'table.move returns the same = table')
+test:ok(t[1] = =3D=3D t[2], 'table.move is correct')

-test:ok(true)
os.exit(test:check() and 0 or 1)
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


+
+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 {}

local ffi =3D = require('ffi')
local tap =3D require('tap')
+local bc =3D require('jit.bc')

ffi.cdef([[
 int setenv(const char *name, = const char *value, int overwrite);
]])

+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?

Fixed. Branch is force-pushed.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
diff --git = a/test/tarantool-tests/utils.lua = b/test/tarantool-tests/utils.lua
index 61d4de7a..57932c5d 100644
--- a/test/tarantool-tests/utils.lua
+++ = b/test/tarantool-tests/utils.lua
@@ -8,7 +8,7 @@ ffi.cdef([[
  int setenv(const char *name, const char *value, = int overwrite);
]])

-local function noop() end
+local function empty() end

local function luacmd(args)
  -- arg[-1] is guaranteed to be not nil.
@@ -101,11 = +101,11 @@ function M.hasbc(f, bytecode)
    write =3D function(out, = line)
      if line:match(bytecode) = then
        hasbc =3D = true
- =        out.write =3D noop
+ =        out.write =3D empty
      end
    end,
-    flush =3D noop,
- =    close =3D noop,
+    flush =3D empty,
+ =    close =3D empty,
  }
  bc.dump(f, out)
  return hasbc
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D


+
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

+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
-- 
2.31.0



-- Best = regards,
Sergey = Kaplun

= --Apple-Mail=_14BC090F-1872-41B0-9BDA-F5D3B786CD76--