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/ <https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guide/>

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                               |  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”)
> 
> 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.
> 
> ===================================================================
> 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 = require("tap")
> -local utils = require("utils")
> +local tap = require('tap')
> +local utils = require('utils')
> 
> -local test = tap.test("gh-6084-missed-carg1-in-bctsetr-fallback")
> +local test = 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.
> 
> ===================================================================
> 
> Side note: we should document our LuaJIT codestyle...
> 
>> 
>>> +
>>> +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.
> 
> Fixed. Branch is force-pushed.
> 
> ===================================================================
> 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 = require('tap')
> local utils = require('utils')
> 
> local test = 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 = {1}
> +local res = table.move(t, 1, 1, 2)
> +test:ok(t == res, 'table.move returns the same table')
> +test:ok(t[1] == t[2], 'table.move is correct')
> 
> -test:ok(true)
> os.exit(test:check() and 0 or 1)
> ===================================================================
> 
>> 
>>> +
>>> +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’?
> 
> Fixed. Branch is force-pushed.
> 
> ===================================================================
> 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 = function(out, line)
>       if line:match(bytecode) then
>         hasbc = true
> -        out.write = noop
> +        out.write = empty
>       end
>     end,
> -    flush = noop,
> -    close = noop,
> +    flush = empty,
> +    close = empty,
>   }
>   bc.dump(f, out)
>   return hasbc
> ===================================================================
> 
>> 
>>> +
>>> 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
>>> 
>> 
> 
> -- 
> Best regards,
> Sergey Kaplun