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

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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