Tarantool development patches archive
 help / color / mirror / Atom feed
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
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
> 


  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 \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git