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 172CB6EC55; Sat, 12 Jun 2021 16:10:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 172CB6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1623503434; bh=G3XTwgrP2vrCfi29i2A9hkxlH9Co9Axq1N6Ta/CXII8=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=PFR2YLy63D9Y7eg8eEIu1U8dB55OTa/AaCn7/UmM7bthl0sRQ2KcopMu6/gJsAfE+ PgxGtMsXd5lexWzPvSd2yP1LEQOiB7jIy7bbml/4rdTrUNkjWItYbIz+2oOxmU+DHx T8ofWoW51eAnV8NT1g6g1GzsHNuWA5MHzPIab8FQ= Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 1F4DF6EC55 for ; Sat, 12 Jun 2021 16:10:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1F4DF6EC55 Received: by smtp58.i.mail.ru with esmtpa (envelope-from ) id 1ls3PD-0000Oq-Ba; Sat, 12 Jun 2021 16:10:31 +0300 Date: Sat, 12 Jun 2021 16:09:18 +0300 To: Igor Munkin , tarantool-patches@dev.tarantool.org Message-ID: References: <51e2abaf644791331a077cab0852aa54d04941ff.1621859367.git.skaplun@tarantool.org> <20210610135119.GX3944@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D5B0DA836B685C549A9F97C297FFF2C725C7934AD8E7B4B9182A05F5380850407BCD9BAFD7C388E1C9DC4FB7E7669B7C32A92D46FC59A76129D924E6CBEF0DC0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE72E4E5201E1C2E308EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637922DC6CAABE3528D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8481ECBABC304BD483CB17041C549D103117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B65D56369A3576CBA5089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5CA7E3ACAD3735FD3EA58553779A6D801FF213A880A04D797D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EA882B598A2098115A792DB242580C1DC17E77169E2FF702EF410C1C753369A211DAE58694D71FF11D7E09C32AA3244C636FAD66C5795C9FF5E850EB9F2829D2E3D93501275E802F729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojlI+L0Np+EkjoJMxzxNLmHw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB406BFDCEA3B335C909BDF02E7C8AD75AAC8AC15759E7DD62EF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, again! I've added some minor updates to the commit message and comments. Branch is force-pushed. On 11.06.21, Sergey Kaplun via Tarantool-patches wrote: > Hi, Igor! > > Thanks for the review! > > On 10.06.21, Igor Munkin wrote: > > Sergey, > > > > Thanks for the patch! LGTM, with several nits below. > > > > On 24.05.21, Sergey Kaplun wrote: > > > From: Mike Pall > > > > > > 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. > > > > Minor: IMHO, it's worth to explicitly mention the value that need to be > > set before the call (strictly saying CARG1 is set by VM, but this is a > > wrong value). Feel free to ignore. > > Update commit message to the following: > > =================================================================== > ARM, ARM64, PPC: Fix TSETR fallback. > > 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 to lua_State 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 > =================================================================== Clarified the commit message as the following: =================================================================== ARM, ARM64, PPC: Fix TSETR fallback. 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 to currently executed Lua thread 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 =================================================================== > > Branch is force-pushed. > > > > > > 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 | 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 > > > > > > > > > > > > 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 = require("tap") > > > +local utils = require("utils") > > > + > > > +local test = tap.test("gh-6084-missed-carg1-in-bctsetr-fallback") > > > +test:plan(1) > > > + > > > +-- Bytecode TSETR appears only in built-ins libraries, when doing > > > > Minor: It's worth to use 'XXX:' here. > > > > > +-- 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 > > > > Ditto. > > > > > +-- 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 > > > > Typo: s/Empty table has asize equals 0/Empty table asize equals 0/. > > > > > +-- nil) to the field by index 1 > 0, to fallback inside TSETR. > > > +table.move({}, 1, 1, 1) > > > > Side note: Totally agree with Sergos; Seen the changes on the branch. > > Fixed comment-related comments. See the iterative patch below: > > =================================================================== > 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 95bf3bd7..04b129d8 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 > @@ -4,20 +4,20 @@ local utils = require('utils') > local test = tap.test('gh-6084-missed-carg1-in-bctsetr-fallback') > test:plan(2) > > --- 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. > +-- XXX: 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 Fix line underfullness here (in LaTEX terms). =================================================================== 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 04b129d8..86bbabe3 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 @@ -5,8 +5,8 @@ local test = tap.test('gh-6084-missed-carg1-in-bctsetr-fallback') test:plan(2) -- XXX: 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 +-- 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 =================================================================== > +-- 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 > +-- XXX: 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')) > > --- `t` table has asize equals 1. Just copy its first element (1) > +-- `t` table asize equals 1. Just copy its first element (1) > -- to the field by index 2 > 1, to fallback inside TSETR. > local t = {1} > local res = table.move(t, 1, 1, 2) > =================================================================== > > > > > > + > > > +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 = {} > > > > > > local ffi = require('ffi') > > > local tap = require('tap') > > > +local bc = require('jit.bc') > > > > > > ffi.cdef([[ > > > int setenv(const char *name, const char *value, int overwrite); > > > ]]) > > > > > > +local function noop() end > > > > This is a dummy function that is only required by , so move the > > helper closer to it. I would even suggest to move it directly to > > , or even use `function() end` three times, but this is not > > our style ;) > > Moved to `M.hasbc()`. > See the iterative patch below: > > =================================================================== > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua > index 57932c5d..5bd42b30 100644 > --- a/test/tarantool-tests/utils.lua > +++ b/test/tarantool-tests/utils.lua > @@ -8,8 +8,6 @@ ffi.cdef([[ > int setenv(const char *name, const char *value, int overwrite); > ]]) > > -local function empty() end > - > local function luacmd(args) > -- arg[-1] is guaranteed to be not nil. > local idx = -2 > @@ -95,6 +93,7 @@ end > function M.hasbc(f, bytecode) > assert(type(f) == 'function', 'argument #1 should be a function') > assert(type(bytecode) == 'string', 'argument #2 should be a string') > + local function empty() end > local hasbc = false > -- Check the bytecode entry line by line. > local out = { > =================================================================== > > > > > > + > > > local function luacmd(args) > > > -- arg[-1] is guaranteed to be not nil. > > > local idx = -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) == 'function', 'argument #1 should be a function') > > > + assert(type(bytecode) == 'string', 'argument #2 should be a string') > > > + local hasbc = false > > > + -- Check the bytecode entry line by line. > > > + local out = { > > > + write = function(out, line) > > > + if line:match(bytecode) then > > > + hasbc = true > > > + out.write = noop > > > + end > > > + end, > > > + flush = noop, > > > + close = noop, > > > > Minor: This is excess for this function, since it doesn't close the > > stream explicitly. Feel free to ignore. > > I've added this for consistency. Ignoring for now. > > > > > > + } > > > + bc.dump(f, out) > > > + return hasbc > > > +end > > > + > > > return M > > > -- > > > 2.31.0 > > > > > > > -- > > Best regards, > > IM > > -- > Best regards, > Sergey Kaplun -- Best regards, Sergey Kaplun