[Tarantool-patches] [PATCH luajit 1/4] ARM, ARM64, PPC: Fix TSETR fallback.

[Igor Munkin]

Sergey,

Thanks for the patch! LGTM, with several nits below.

On 24.05.21, Sergey Kaplun 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.

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.

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

<snipped>

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

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.

> +
> +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 <M.hasbc>, so move the
helper closer to it. I would even suggest to move it directly to
<M.hasbc>, or even use `function() end` three times, but this is not
our style ;)

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

> +  }
> +  bc.dump(f, out)
> +  return hasbc
> +end
> +
>  return M
> -- 
> 2.31.0
> 

-- 
Best regards,
IM