[Tarantool-patches] [PATCH luajit 0/2 v8] introduce cli for tools

[Tarantool-patches] [PATCH luajit 0/2 v8] introduce cli for tools

[Maxim Kokryashkin via Tarantool-patches]

This first patch requires additional changes for the Tarantool proxying.
These changes are done in the PR. The first patch has the test disabled for
Tarantool, so LuaJIT's integrational testing behaves properly. However,
the second patch in this series enables this test for Tarantool, and it
should be applied after the LuaJIT bump with the first patch.

To ensure the test results without the second patch, I've made and
additional branch just for the LuaJIT integrational testing, that
doesn't have the second patch. All of the required links are located
down below.

PR: https://github.com/tarantool/tarantool/pull/8002
Branch (whole series): https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse-tnt
Branch (the first patch only): https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse

Maxim Kokryashkin (2):
  tools: add cli flag to run profile dump parsers
  test: don't skip tool CLI flag for tarantool

 Makefile.original                             |  20 ---
 src/luajit.c                                  |  41 +++++-
 .../gh-5688-tool-cli-flag.test.lua            | 127 ++++++++++++++++++
 tools/CMakeLists.txt                          |  73 ----------
 tools/luajit-parse-memprof.in                 |   6 -
 tools/luajit-parse-sysprof.in                 |   6 -
 tools/memprof.lua                             |   4 +-
 tools/sysprof.lua                             |  16 ++-
 8 files changed, 183 insertions(+), 110 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
 delete mode 100755 tools/luajit-parse-memprof.in
 delete mode 100644 tools/luajit-parse-sysprof.in

--
2.41.0

[Tarantool-patches] [PATCH luajit 1/2 v8] tools: add cli flag to run profile dump parsers

[Maxim Kokryashkin via Tarantool-patches]

It is really inconvenient to use a standalone shell script to parse
binary dumps from profilers. That is why this commit introduces a
CLI flag for tools to the LuaJIT, so now it is possible to parse
a memprof dump as simple as:
```
luajit -tm memprof.bin
luajit -tm --leak-only memprof.bin
```

And the sysprof too:
```
luajit -ts sysprof.bin
luajit -ts --split sysprof.bin
```

The `luajit-parse-memprof` and `luajit-parse-sysprof` are purged
as a result of this changes.

Closes tarantool/tarantool#5688
---
This patch requires additional changes for tarantool proxying,
that are done in the PR.
 Makefile.original                             |  20 ---
 src/luajit.c                                  |  41 +++++-
 .../gh-5688-tool-cli-flag.test.lua            | 130 ++++++++++++++++++
 tools/CMakeLists.txt                          |  73 ----------
 tools/luajit-parse-memprof.in                 |   6 -
 tools/luajit-parse-sysprof.in                 |   6 -
 tools/memprof.lua                             |   4 +-
 tools/sysprof.lua                             |  16 ++-
 8 files changed, 186 insertions(+), 110 deletions(-)
 create mode 100644 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
 delete mode 100755 tools/luajit-parse-memprof.in
 delete mode 100644 tools/luajit-parse-sysprof.in

diff --git a/Makefile.original b/Makefile.original
index 0c92df9e..10836f7d 100644
--- a/Makefile.original
+++ b/Makefile.original
@@ -58,7 +58,6 @@ INSTALL_DYLIBSHORT2= libluajit-$(ABIVER).$(MAJVER).dylib
 INSTALL_DYLIBNAME= libluajit-$(ABIVER).$(MAJVER).$(MINVER).$(RELVER).dylib
 INSTALL_PCNAME= luajit.pc
 INSTALL_TMEMPROFNAME= luajit-$(VERSION)-parse-memprof
-INSTALL_TMEMPROFSYMNAME= luajit-parse-memprof
 
 INSTALL_STATIC= $(INSTALL_LIB)/$(INSTALL_ANAME)
 INSTALL_DYN= $(INSTALL_LIB)/$(INSTALL_SONAME)
@@ -68,7 +67,6 @@ INSTALL_T= $(INSTALL_BIN)/$(INSTALL_TNAME)
 INSTALL_TSYM= $(INSTALL_BIN)/$(INSTALL_TSYMNAME)
 INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME)
 INSTALL_TMEMPROF= $(INSTALL_BIN)/$(INSTALL_TMEMPROFNAME)
-INSTALL_TMEMPROFSYM= $(INSTALL_BIN)/$(INSTALL_TMEMPROFSYMNAME)
 
 INSTALL_DIRS= $(INSTALL_BIN) $(INSTALL_LIB) $(INSTALL_INC) $(INSTALL_MAN) \
   $(INSTALL_PKGCONFIG) $(INSTALL_JITLIB) $(INSTALL_LMOD) $(INSTALL_CMOD) \
@@ -87,8 +85,6 @@ UNINSTALL= $(RM)
 LDCONFIG= ldconfig -n
 SED_PC= sed -e "s|@LUAJIT_PC_PREFIX@|$(PREFIX)|" \
             -e "s|@LUAJIT_PC_MULTILIB@|$(MULTILIB)|"
-SED_TMEMPROF= sed -e "s|@LUAJIT_TOOLS_DIR@|$(INSTALL_TOOLSLIB)|" \
-                  -e "s|@LUAJIT_TOOLS_BIN@|$(INSTALL_T)|"
 
 FILE_T= luajit
 FILE_A= libluajit.a
@@ -103,7 +99,6 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
 FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
 FILES_MEMPROFLIB= parse.lua humanize.lua
 FILES_TOOLSLIB= memprof.lua
-FILE_TMEMPROF= luajit-parse-memprof
 
 ifeq (,$(findstring Windows,$(OS)))
   HOST_SYS:= $(shell uname -s)
@@ -148,16 +143,12 @@ install: $(INSTALL_DEP)
 	cd tools/utils && $(INSTALL_F) $(FILES_UTILSLIB) $(INSTALL_UTILSLIB)
 	cd tools/memprof && $(INSTALL_F) $(FILES_MEMPROFLIB) $(INSTALL_MEMPROFLIB)
 	cd tools && $(INSTALL_F) $(FILES_TOOLSLIB) $(INSTALL_TOOLSLIB)
-	cd tools && $(SED_TMEMPROF) $(FILE_TMEMPROF).in > $(FILE_TMEMPROF) && \
-	  $(INSTALL_X) $(FILE_TMEMPROF) $(INSTALL_TMEMPROF) && \
-	  $(RM) $(FILE_TMEMPROF)
 	@echo "==== Successfully installed LuaJIT $(VERSION) to $(PREFIX) ===="
 	@echo ""
 	@echo "Note: the development releases deliberately do NOT install a symlink for luajit"
 	@echo "You can do this now by running these commands (with sudo):"
 	@echo ""
 	@echo "  $(SYMLINK) $(INSTALL_TNAME) $(INSTALL_TSYM)"
-	@echo "  $(SYMLINK) $(INSTALL_TMEMPROFNAME) $(INSTALL_TMEMPROFSYM)"
 	@echo ""
 
 
@@ -190,19 +181,8 @@ amalg: tools
 	$(MAKE) -C src -f Makefile.original amalg
 
 clean:
-	$(RM) tools/$(FILE_TMEMPROF)
 	$(MAKE) -C src -f Makefile.original clean
 
-tools: tools/$(FILE_TMEMPROF)
-
-# FIXME: This is an ugly hack to manually configure an auxiliary
-# tools/luajit-parse-memprof. This file should go away in scope of
-# https://github.com/tarantool/tarantool/issues/5688.
-tools/$(FILE_TMEMPROF): src/luajit
-	@sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \
-	     -e "s|@LUAJIT_TOOLS_BIN@|$(realpath src/luajit)|" \
-	     $@.in > $@
-	@chmod +x $@
 
 .PHONY: all install amalg clean tools
 
diff --git a/src/luajit.c b/src/luajit.c
index 1ca24301..e9ed89bc 100644
--- a/src/luajit.c
+++ b/src/luajit.c
@@ -72,6 +72,7 @@ static void print_usage(void)
   "  -O[opt]   Control LuaJIT optimizations.\n"
   "  -i        Enter interactive mode after executing " LUA_QL("script") ".\n"
   "  -v        Show version information.\n"
+  "  -t<cmd>   Execute tool.\n"
   "  -E        Ignore environment variables.\n"
   "  --        Stop handling options.\n"
   "  -         Execute stdin and stop handling options.\n", stderr);
@@ -361,6 +362,34 @@ static int dojitcmd(lua_State *L, const char *cmd)
   return runcmdopt(L, opt ? opt+1 : opt);
 }
 
+static int runtoolcmd(lua_State *L, const char *tool_name)
+{
+  lua_getglobal(L, "require");
+  lua_pushstring(L, tool_name);
+  if (lua_pcall(L, 1, 1, 0)) {
+    const char *msg = lua_tostring(L, -1);
+    if (msg && !strncmp(msg, "module ", 7))
+      l_message(progname,
+        "unknown luaJIT command or tools not installed");
+    return 1;
+  }
+  lua_getglobal(L, "arg");
+  return report(L, lua_pcall(L, 1, 1, 0));
+}
+
+static int dotoolcmd(lua_State *L, const char *cmd)
+{
+  switch (cmd[0]) {
+  case 'm':
+    return runtoolcmd(L, "memprof");
+  case 's':
+    return runtoolcmd(L, "sysprof");
+  default:
+    break;
+  }
+  return -1;
+}
+
 /* Optimization flags. */
 static int dojitopt(lua_State *L, const char *opt)
 {
@@ -398,6 +427,7 @@ static int dobytecode(lua_State *L, char **argv)
 #define FLAGS_EXEC		4
 #define FLAGS_OPTION		8
 #define FLAGS_NOENV		16
+#define FLAGS_TOOL		32
 
 static int collectargs(char **argv, int *flags)
 {
@@ -419,6 +449,11 @@ static int collectargs(char **argv, int *flags)
       notail(argv[i]);
       *flags |= FLAGS_VERSION;
       break;
+    case 't':
+      *flags |= FLAGS_TOOL;
+      if (argv[i][2] == '\0') return -1;
+      if (argv[i + 1] == NULL) return -1;
+      return i + 1;
     case 'e':
       *flags |= FLAGS_EXEC;
     case 'j':  /* LuaJIT extension */
@@ -474,6 +509,10 @@ static int runargs(lua_State *L, char **argv, int argn)
 	return 1;
       break;
       }
+    case 't': { /* Tarantool's fork extension. */
+      const char *cmd = argv[i] + 2;
+      return dotoolcmd(L, cmd) != LUA_OK;
+    }
     case 'O':  /* LuaJIT extension. */
       if (dojitopt(L, argv[i] + 2))
 	return 1;
@@ -535,7 +574,7 @@ static int pmain(lua_State *L)
   luaL_openlibs(L);
   lua_gc(L, LUA_GCRESTART, -1);
 
-  createargtable(L, argv, s->argc, argn);
+  createargtable(L, argv, s->argc, (flags & FLAGS_TOOL) ? argn - 1 : argn);
 
   if (!(flags & FLAGS_NOENV)) {
     s->status = handle_luainit(L);
diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
new file mode 100644
index 00000000..b91902fd
--- /dev/null
+++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
@@ -0,0 +1,130 @@
+local tap = require('tap')
+local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
+  ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                               jit.arch ~= 'x64',
+  ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
+  -- XXX: Tarantool integration is required to run this test properly.
+  -- luacheck: no global
+  ['No profile tools CLI option integration'] = _TARANTOOL,
+})
+
+test:plan(3)
+
+jit.off()
+jit.flush()
+
+local table_new = require('table.new')
+local utils = require('utils')
+
+local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin')
+local TMP_BINFILE_MEMPROF = utils.tools.profilename('memprofdata.tmp.bin')
+local TMP_BINFILE_SYSPROF = utils.tools.profilename('sysprofdata.tmp.bin')
+
+local EXECUTABLE = utils.exec.luacmd(arg)
+local MEMPROF_PARSER = EXECUTABLE .. ' -tm '
+local SYSPROF_PARSER = EXECUTABLE .. ' -ts '
+
+local SUPPRESS_OUTPUT = ' > /dev/null 2>&1'
+
+local TABLE_SIZE = 20
+
+local SMOKE_CMD_SET = {
+  {
+    cmd = EXECUTABLE .. ' -t ' .. BAD_PATH,
+    expected = false,
+  },
+  {
+    cmd = EXECUTABLE .. ' -ta ' .. BAD_PATH,
+    expected = false,
+  },
+}
+
+local MEMPROF_CMD_SET = {
+  {
+    cmd = MEMPROF_PARSER .. BAD_PATH,
+    expected = false,
+  },
+  {
+    cmd = MEMPROF_PARSER .. TMP_BINFILE_MEMPROF,
+    expected = true,
+  },
+  {
+    cmd = MEMPROF_PARSER .. ' --wrong ' .. TMP_BINFILE_MEMPROF,
+    expected = false,
+  },
+  {
+    cmd = MEMPROF_PARSER .. ' --leak-only ' .. TMP_BINFILE_MEMPROF,
+    expected = true,
+  },
+}
+
+local SYSPROF_CMD_SET = {
+  {
+    cmd = SYSPROF_PARSER .. BAD_PATH,
+    expected = false,
+  },
+  {
+    cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF,
+    expected = true,
+  },
+  {
+    cmd = SYSPROF_PARSER .. ' --wrong ' .. TMP_BINFILE_SYSPROF,
+    expected = false,
+  },
+  {
+    cmd = SYSPROF_PARSER .. ' --split ' .. TMP_BINFILE_SYSPROF,
+    expected = true,
+  },
+}
+
+local function memprof_payload()
+  local _ = table_new(TABLE_SIZE, 0)
+   _ = nil
+  collectgarbage()
+end
+
+local function sysprof_payload()
+  local function fib(n)
+    if n <= 1 then
+      return n
+    end
+    return fib(n - 1) + fib(n - 2)
+  end
+  return fib(32)
+end
+
+local function generate_profiler_output(opts, payload, profiler)
+  local res, err = profiler.start(opts)
+  -- Should start succesfully.
+  assert(res, err)
+
+  payload()
+
+  res, err = profiler.stop()
+  -- Should stop succesfully.
+  assert(res, err)
+end
+
+local function tool_test_case(case_name, cmd_set)
+  test:test(case_name, function(subtest)
+    subtest:plan(#cmd_set)
+
+    for idx = 1, #cmd_set do
+      local result = os.execute(cmd_set[idx].cmd .. SUPPRESS_OUTPUT) == 0
+      subtest:ok(result == cmd_set[idx].expected, cmd_set[idx].cmd)
+    end
+  end)
+end
+
+tool_test_case('smoke', SMOKE_CMD_SET)
+
+generate_profiler_output(TMP_BINFILE_MEMPROF, memprof_payload, misc.memprof)
+tool_test_case('memprof parsing tool', MEMPROF_CMD_SET)
+
+local sysprof_opts = { mode = 'C', path = TMP_BINFILE_SYSPROF }
+generate_profiler_output(sysprof_opts, sysprof_payload, misc.sysprof)
+tool_test_case('sysprof parsing tool', SYSPROF_CMD_SET)
+
+os.remove(TMP_BINFILE_MEMPROF)
+os.remove(TMP_BINFILE_SYSPROF)
+test:done(true)
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index dd7ec6bd..b7aeb472 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -11,22 +11,7 @@ set(LUAJIT_TOOLS_DEPS)
 if(LUAJIT_DISABLE_MEMPROF)
   message(STATUS "LuaJIT memory profiler support is disabled")
 else()
-  # XXX: Can use genex here since the value need to be evaluated
-  # at the configuration phase. Fortunately, we know the exact
-  # path where LuaJIT binary is located.
-  set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME})
-  set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR})
-  # XXX: Unfortunately, there is no convenient way to set
-  # particular permissions to the output file via CMake.
-  # Furthermore, I even failed to copy the given file to the same
-  # path to change its permissions. After looking at the docs, I
-  # realized that the valid solution would be too monstrous for
-  # such a simple task. As a result I've made the template itself
-  # executable, so the issue is resolved.
-  configure_file(luajit-parse-memprof.in luajit-parse-memprof @ONLY ESCAPE_QUOTES)
-
   add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
-    luajit-parse-memprof
     memprof/humanize.lua
     memprof/parse.lua
     memprof.lua
@@ -66,27 +51,6 @@ else()
       WORLD_READ
     COMPONENT tools-parse-memprof
   )
-  install(CODE
-    # XXX: The auxiliary script needs to be configured for to be
-    # used in repository directly. Furthermore, it needs to be
-    # reconfigured prior to its installation. The temporary
-    # <configure_file> output is stored to the project build
-    # directory and removed later after being installed. This
-    # script will have gone as a result of the issue:
-    # https://github.com/tarantool/tarantool/issues/5688.
-    "
-      set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME})
-      set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR})
-      configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-memprof.in
-        ${PROJECT_BINARY_DIR}/luajit-parse-memprof @ONLY ESCAPE_QUOTES)
-      file(INSTALL ${PROJECT_BINARY_DIR}/luajit-parse-memprof
-        DESTINATION ${CMAKE_INSTALL_PREFIX}/bin
-        USE_SOURCE_PERMISSIONS
-      )
-      file(REMOVE ${PROJECT_BINARY_DIR}/luajit-parse-memprof)
-    "
-    COMPONENT tools-parse-memprof
-  )
 endif()
 
 
@@ -94,23 +58,7 @@ endif()
 if(LUAJIT_DISABLE_SYSPROF)
   message(STATUS "LuaJIT system profiler support is disabled")
 else()
-  # XXX: Can use genex here since the value need to be evaluated
-  # at the configuration phase. Fortunately, we know the exact
-  # path where LuaJIT binary is located.
-  set(LUAJIT_TOOLS_BIN ${LUAJIT_BINARY_DIR}/${LUAJIT_CLI_NAME})
-  set(LUAJIT_TOOLS_DIR ${CMAKE_CURRENT_SOURCE_DIR})
-  # XXX: Unfortunately, there is no convenient way to set
-  # particular permissions to the output file via CMake.
-  # Furthermore, I even failed to copy the given file to the same
-  # path to change its permissions. After looking at the docs, I
-  # realized that the valid solution would be too monstrous for
-  # such a simple task. As a result I've made the template itself
-  # executable, so the issue is resolved.
-  configure_file(luajit-parse-sysprof.in luajit-parse-sysprof @ONLY ESCAPE_QUOTES)
-
-
   add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
-    luajit-parse-sysprof
     sysprof/parse.lua
     sysprof/collapse.lua
     sysprof.lua
@@ -148,27 +96,6 @@ else()
       WORLD_READ
     COMPONENT tools-parse-sysprof
   )
-  install(CODE
-    # XXX: The auxiliary script needs to be configured for to be
-    # used in repository directly. Furthermore, it needs to be
-    # reconfigured prior to its installation. The temporary
-    # <configure_file> output is stored to the project build
-    # directory and removed later after being installed. This
-    # script will have gone as a result of the issue:
-    # https://github.com/tarantool/tarantool/issues/5688.
-    "
-      set(LUAJIT_TOOLS_BIN ${CMAKE_INSTALL_PREFIX}/bin/${LUAJIT_CLI_NAME})
-      set(LUAJIT_TOOLS_DIR ${CMAKE_INSTALL_PREFIX}/${LUAJIT_DATAROOTDIR})
-      configure_file(${CMAKE_CURRENT_SOURCE_DIR}/luajit-parse-sysprof.in
-        ${PROJECT_BINARY_DIR}/luajit-parse-sysprof @ONLY ESCAPE_QUOTES)
-      file(INSTALL ${PROJECT_BINARY_DIR}/luajit-parse-sysprof
-        DESTINATION ${CMAKE_INSTALL_PREFIX}/bin
-        USE_SOURCE_PERMISSIONS
-      )
-      file(REMOVE ${PROJECT_BINARY_DIR}/luajit-parse-sysprof)
-    "
-    COMPONENT tools-parse-sysprof
-  )
 endif()
 
 add_custom_target(LuaJIT-tools DEPENDS ${LUAJIT_TOOLS_DEPS})
diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in
deleted file mode 100755
index 8867202a..00000000
--- a/tools/luajit-parse-memprof.in
+++ /dev/null
@@ -1,6 +0,0 @@
-#!/bin/bash
-#
-# Launcher for memprof parser.
-
-LUA_PATH="@LUAJIT_TOOLS_DIR@/?.lua;;" \
-	@LUAJIT_TOOLS_BIN@ @LUAJIT_TOOLS_DIR@/memprof.lua $@
diff --git a/tools/luajit-parse-sysprof.in b/tools/luajit-parse-sysprof.in
deleted file mode 100644
index 2be25eb3..00000000
--- a/tools/luajit-parse-sysprof.in
+++ /dev/null
@@ -1,6 +0,0 @@
-#!/bin/bash
-#
-# Launcher for sysprof parser.
-
-LUA_PATH="@LUAJIT_TOOLS_DIR@/?.lua;;" \
-  @LUAJIT_TOOLS_BIN@ @LUAJIT_TOOLS_DIR@/sysprof.lua $@
diff --git a/tools/memprof.lua b/tools/memprof.lua
index cf66dd9e..8df1630a 100644
--- a/tools/memprof.lua
+++ b/tools/memprof.lua
@@ -107,7 +107,9 @@ local function dump(inputfile)
   local dheap = process.form_heap_delta(events, symbols)
   view.leak_info(dheap)
   view.aliases(symbols)
-  os.exit(0)
+  -- XXX: The second argument is required to properly close Lua
+  -- universe (i.e. invoke <lua_close> before exiting).
+  os.exit(0, true)
 end
 
 -- XXX: When this script is used as a preloaded module by an
diff --git a/tools/sysprof.lua b/tools/sysprof.lua
index 1afab195..6ca7f2f2 100644
--- a/tools/sysprof.lua
+++ b/tools/sysprof.lua
@@ -107,13 +107,23 @@ local function dump(inputfile)
 
   traverse_calltree(calltree, '')
 
-  os.exit(0)
+  -- XXX: The second argument is required to properly close Lua
+  -- universe (i.e. invoke <lua_close> before exiting).
+  os.exit(0, true)
+end
+
+-- XXX: When this script is used as a preloaded module by an
+-- application, it should return one function for correct parsing
+-- of command line flags like --leak-only and dumping profile
+-- info.
+local function dump_wrapped(...)
+  return dump(parseargs(...))
 end
 
 -- FIXME: this script should be application-independent.
 local args = {...}
 if #args == 1 and args[1] == "sysprof" then
-  return dump
+  return dump_wrapped
 else
-  dump(parseargs(args))
+  dump_wrapped(args)
 end
-- 
2.41.0

[Tarantool-patches] [PATCH luajit 2/2 v8] test: don't skip tool CLI flag for tarantool

[Maxim Kokryashkin via Tarantool-patches]

That skipcond was introduced to overcome the obstacles
of LuaJIT's integrational testing in Tarantool. Since
the required patch is now in the Tarantool master, this
skipcond is now unnecessary.

Related to tarantool/tarantool#5688
---
 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
index b91902fd..33627ee2 100644
--- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
@@ -3,9 +3,6 @@ local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
   ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
                                                jit.arch ~= 'x64',
   ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux',
-  -- XXX: Tarantool integration is required to run this test properly.
-  -- luacheck: no global
-  ['No profile tools CLI option integration'] = _TARANTOOL,
 })

 test:plan(3)
--
2.41.0

Re: [Tarantool-patches] [PATCH luajit 1/2 v8] tools: add cli flag to run profile dump parsers

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.

On 25.08.23, Maxim Kokryashkin wrote:
> It is really inconvenient to use a standalone shell script to parse
> binary dumps from profilers. That is why this commit introduces a
> CLI flag for tools to the LuaJIT, so now it is possible to parse

Typo? s/for tools to the/for tools in the/

> a memprof dump as simple as:
> ```
> luajit -tm memprof.bin
> luajit -tm --leak-only memprof.bin
> ```
> 
> And the sysprof too:
> ```
> luajit -ts sysprof.bin
> luajit -ts --split sysprof.bin
> ```
> 
> The `luajit-parse-memprof` and `luajit-parse-sysprof` are purged
> as a result of this changes.

Typo: s/this/these/

> 
> Closes tarantool/tarantool#5688
> ---
> This patch requires additional changes for tarantool proxying,
> that are done in the PR.
>  Makefile.original                             |  20 ---
>  src/luajit.c                                  |  41 +++++-
>  .../gh-5688-tool-cli-flag.test.lua            | 130 ++++++++++++++++++
>  tools/CMakeLists.txt                          |  73 ----------
>  tools/luajit-parse-memprof.in                 |   6 -
>  tools/luajit-parse-sysprof.in                 |   6 -
>  tools/memprof.lua                             |   4 +-
>  tools/sysprof.lua                             |  16 ++-
>  8 files changed, 186 insertions(+), 110 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
>  delete mode 100755 tools/luajit-parse-memprof.in
>  delete mode 100644 tools/luajit-parse-sysprof.in
> 
> diff --git a/Makefile.original b/Makefile.original
> index 0c92df9e..10836f7d 100644
> --- a/Makefile.original
> +++ b/Makefile.original
> @@ -58,7 +58,6 @@ INSTALL_DYLIBSHORT2= libluajit-$(ABIVER).$(MAJVER).dylib
>  INSTALL_DYLIBNAME= libluajit-$(ABIVER).$(MAJVER).$(MINVER).$(RELVER).dylib
>  INSTALL_PCNAME= luajit.pc
>  INSTALL_TMEMPROFNAME= luajit-$(VERSION)-parse-memprof

I suppose, that this should be dropped too.

> -INSTALL_TMEMPROFSYMNAME= luajit-parse-memprof
>  
>  INSTALL_STATIC= $(INSTALL_LIB)/$(INSTALL_ANAME)
>  INSTALL_DYN= $(INSTALL_LIB)/$(INSTALL_SONAME)
> @@ -68,7 +67,6 @@ INSTALL_T= $(INSTALL_BIN)/$(INSTALL_TNAME)
>  INSTALL_TSYM= $(INSTALL_BIN)/$(INSTALL_TSYMNAME)
>  INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME)
>  INSTALL_TMEMPROF= $(INSTALL_BIN)/$(INSTALL_TMEMPROFNAME)

As far as this one.

> -INSTALL_TMEMPROFSYM= $(INSTALL_BIN)/$(INSTALL_TMEMPROFSYMNAME)

<snipped>

> @@ -103,7 +99,6 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \
>  FILES_UTILSLIB= avl.lua bufread.lua symtab.lua
>  FILES_MEMPROFLIB= parse.lua humanize.lua

I noticed that this variable misses the <process.lua> recently
introduced. Also, there are no libs to install via sysprof too. I
suggest to fix this misbehaviour within the other patch (not even in
this patchset).

>  FILES_TOOLSLIB= memprof.lua
> -FILE_TMEMPROF= luajit-parse-memprof
>  

<snipped>

> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..e9ed89bc 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c

<snipped>

> +static int runtoolcmd(lua_State *L, const char *tool_name)
> +{
> +  lua_getglobal(L, "require");
> +  lua_pushstring(L, tool_name);
> +  if (lua_pcall(L, 1, 1, 0)) {
> +    const char *msg = lua_tostring(L, -1);
> +    if (msg && !strncmp(msg, "module ", 7))
> +      l_message(progname,
> +        "unknown luaJIT command or tools not installed");

Nit: There is no need for new line here.

Also, I strongly believe that we should report an error in the case,
when there is another error reason. Just to avoid silent failures.

> +    return 1;
> +  }
> +  lua_getglobal(L, "arg");
> +  return report(L, lua_pcall(L, 1, 1, 0));
> +}
> +

<snipped>

> diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> new file mode 100644
> index 00000000..b91902fd
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua

<snipped>

> +
> +local function tool_test_case(case_name, cmd_set)
> +  test:test(case_name, function(subtest)
> +    subtest:plan(#cmd_set)
> +
> +    for idx = 1, #cmd_set do
> +      local result = os.execute(cmd_set[idx].cmd .. SUPPRESS_OUTPUT) == 0

Here we got a problem: if sysprof has bad arguments, sysprof reports
nothing -- just silently fails:

| >>> LUA_PATH="tools/?.lua;;" src/luajit -ts /tmp/memprof.bin
| 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1]
| >>> LUA_PATH="tools/?.lua;;" src/luajit -ts asdfl
| 13:29:14 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1]

I suggest to check the error message too.

> +      subtest:ok(result == cmd_set[idx].expected, cmd_set[idx].cmd)
> +    end
> +  end)
> +end
> +
> +tool_test_case('smoke', SMOKE_CMD_SET)
> +
> +generate_profiler_output(TMP_BINFILE_MEMPROF, memprof_payload, misc.memprof)
> +tool_test_case('memprof parsing tool', MEMPROF_CMD_SET)
> +
> +local sysprof_opts = { mode = 'C', path = TMP_BINFILE_SYSPROF }
> +generate_profiler_output(sysprof_opts, sysprof_payload, misc.sysprof)
> +tool_test_case('sysprof parsing tool', SYSPROF_CMD_SET)
> +
> +os.remove(TMP_BINFILE_MEMPROF)
> +os.remove(TMP_BINFILE_SYSPROF)
> +test:done(true)
> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index dd7ec6bd..b7aeb472 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

>    add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS
> -    luajit-parse-memprof
>      memprof/humanize.lua
>      memprof/parse.lua
>      memprof.lua

<memprof/process.lua> is missing here.

> @@ -66,27 +51,6 @@ else()
>        WORLD_READ
>      COMPONENT tools-parse-memprof
>    )

<snipped>

>    add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS
> -    luajit-parse-sysprof
>      sysprof/parse.lua
>      sysprof/collapse.lua
>      sysprof.lua
> @@ -148,27 +96,6 @@ else()
>        WORLD_READ
>      COMPONENT tools-parse-sysprof
>    )

<snipped>

> diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in
> deleted file mode 100755
> index 8867202a..00000000
> --- a/tools/luajit-parse-memprof.in
> +++ /dev/null

<snipped>

> diff --git a/tools/luajit-parse-sysprof.in b/tools/luajit-parse-sysprof.in
> deleted file mode 100644
> index 2be25eb3..00000000
> --- a/tools/luajit-parse-sysprof.in
> +++ /dev/null

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index cf66dd9e..8df1630a 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua

<snipped>

> +-- XXX: When this script is used as a preloaded module by an
> +-- application, it should return one function for correct parsing
> +-- of command line flags like --leak-only and dumping profile
> +-- info.
> +local function dump_wrapped(...)
> +  return dump(parseargs(...))
>  end
>  
>  -- FIXME: this script should be application-independent.
>  local args = {...}
>  if #args == 1 and args[1] == "sysprof" then
> -  return dump
> +  return dump_wrapped
>  else
> -  dump(parseargs(args))
> +  dump_wrapped(args)

Do we ever need this case since there is no direct call to it?
I suppose that this FIXME and if/else branches may be deleted.
Now we may always return the `dump_wrapped()` function.

>  end
> -- 
> 2.41.0
> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit 2/2 v8] test: don't skip tool CLI flag for tarantool

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the patch!
LGTM, as trivial, with a single nit regarding the commit message.

On 25.08.23, Maxim Kokryashkin wrote:
> That skipcond was introduced to overcome the obstacles
> of LuaJIT's integrational testing in Tarantool. Since

Typo: s/integrational/integration/

> the required patch is now in the Tarantool master, this
> skipcond is now unnecessary.
> 
> Related to tarantool/tarantool#5688
> ---

<snipped>


-- 
Best regards,
Sergey Kaplun