From: Sergey Ostanevich via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH luajit 1/4] ARM, ARM64, PPC: Fix TSETR fallback. Date: Wed, 2 Jun 2021 15:04:25 +0300 [thread overview] Message-ID: <225B5D75-E1D0-4137-8E78-FC78EE6952A0@tarantool.org> (raw) In-Reply-To: <51e2abaf644791331a077cab0852aa54d04941ff.1621859367.git.skaplun@tarantool.org> 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 | 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/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/?.lua" > + "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;${PROJECT_SOURCE_DIR}/tools/?.lua\;${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.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") 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(‘tap') against require("tap”) > + > +local test = 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. > + > +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 Name of this one in a patch that messess with bytecodes is confusing. Could it be a simpler one, like ‘empty’? > + > 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, > + } > + bc.dump(f, out) > + return hasbc > +end > + > return M > -- > 2.31.0 >
next prev parent reply other threads:[~2021-06-02 12:04 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-24 13:27 [Tarantool-patches] [PATCH luajit 0/4] Fix LuaJIT tests on aarch64, odroid Sergey Kaplun via Tarantool-patches 2021-05-24 13:27 ` [Tarantool-patches] [PATCH luajit 1/4] ARM, ARM64, PPC: Fix TSETR fallback Sergey Kaplun via Tarantool-patches 2021-06-02 12:04 ` Sergey Ostanevich via Tarantool-patches [this message] 2021-06-04 13:12 ` Sergey Kaplun via Tarantool-patches 2021-06-04 15:33 ` Sergey Ostanevich via Tarantool-patches 2021-06-04 15:39 ` Sergey Kaplun via Tarantool-patches 2021-06-10 13:51 ` Igor Munkin via Tarantool-patches 2021-06-11 8:47 ` Sergey Kaplun via Tarantool-patches 2021-06-12 13:09 ` Sergey Kaplun via Tarantool-patches 2021-05-24 13:27 ` [Tarantool-patches] [PATCH luajit 2/4] test: add skipcond on architectures for memprof Sergey Kaplun via Tarantool-patches 2021-06-02 12:28 ` Sergey Ostanevich via Tarantool-patches 2021-06-04 13:37 ` Sergey Kaplun via Tarantool-patches 2021-06-04 15:36 ` Sergey Ostanevich via Tarantool-patches 2021-06-04 16:18 ` Sergey Kaplun via Tarantool-patches 2021-06-10 13:51 ` Igor Munkin via Tarantool-patches 2021-06-11 8:18 ` Sergey Kaplun via Tarantool-patches 2021-05-24 13:27 ` [Tarantool-patches] [PATCH luajit 3/4] ARM64: Fix xpcall() error case Sergey Kaplun via Tarantool-patches 2021-06-02 12:47 ` Sergey Ostanevich via Tarantool-patches 2021-06-04 13:45 ` Sergey Kaplun via Tarantool-patches 2021-06-10 13:51 ` Igor Munkin via Tarantool-patches 2021-05-24 13:27 ` [Tarantool-patches] [PATCH luajit 4/4] ARM64: Fix xpcall() error case (really) Sergey Kaplun via Tarantool-patches 2021-06-02 14:43 ` Sergey Ostanevich via Tarantool-patches 2021-06-04 13:56 ` Sergey Kaplun via Tarantool-patches 2021-06-10 13:52 ` Igor Munkin via Tarantool-patches 2021-06-11 8:08 ` Sergey Kaplun via Tarantool-patches 2021-06-01 11:11 ` [Tarantool-patches] [PATCH luajit 0/4] Fix LuaJIT tests on aarch64, odroid Igor Munkin via Tarantool-patches 2021-06-12 16:02 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=225B5D75-E1D0-4137-8E78-FC78EE6952A0@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=sergos@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH luajit 1/4] ARM, ARM64, PPC: Fix TSETR fallback.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox