[Tarantool-patches] [PATCH luajit v6] memprof: introduce cli flag to run dump parser

[Tarantool-patches] [PATCH luajit v6] memprof: introduce cli flag to run dump parser

[Maxim Kokryashkin via Tarantool-patches]

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

Closes tarantool/tarantool#5688
---
Changes in v6:
- Simplified the patch.

Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
PR: https://github.com/tarantool/tarantool/pull/8002

 src/luajit.c                                  | 31 ++++++++++-
 .../gh-5688-memprof-cli-flag.test.lua         | 55 +++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua

diff --git a/src/luajit.c b/src/luajit.c
index 1ca24301..81c0f8af 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,24 @@ static int dojitcmd(lua_State *L, const char *cmd)
   return runcmdopt(L, opt ? opt+1 : opt);
 }
 
+static int dotoolcmd(lua_State *L, const char *cmd)
+{
+  if(strcmp(cmd, "m") == 0) {
+    lua_getglobal(L, "require");
+    lua_pushliteral(L, "memprof");
+    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));
+  }
+  return -1;
+}
+
 /* Optimization flags. */
 static int dojitopt(lua_State *L, const char *opt)
 {
@@ -398,6 +417,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 +439,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 +499,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 +564,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-memprof-cli-flag.test.lua b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
new file mode 100644
index 00000000..ba0cad68
--- /dev/null
+++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
@@ -0,0 +1,55 @@
+local tap = require('tap')
+local test = tap.test('gh-5688-cli-for-memprof-parse'):skipcond({
+  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
+                                               jit.arch ~= 'x64',
+  ['Memprof is implemented for Linux only'] = jit.os ~= 'Linux',
+-- XXX: The patch is for LuaJIT only, and it doesn't
+-- work on Tarantool.
+-- luacheck: no global
+  ['No memprof CLI option'] = _TARANTOOL,
+})
+
+test:plan(2)
+
+jit.off()
+jit.flush()
+
+local table_new = require('table.new')
+local utils = require('utils')
+
+local TMP_BINFILE = utils.tools.profilename('memprofdata.tmp.bin')
+local BAD_PATH = utils.tools.profilename('bad-path-tmp.bin')
+local EXECUTABLE = utils.exec.luacmd(arg)
+local TABLE_SIZE = 20
+
+local function default_payload()
+  local _ = table_new(TABLE_SIZE, 0)
+   _ = nil
+  collectgarbage()
+end
+
+local function generate_output(filename, payload)
+  -- Clean up all garbage to avoid pollution of free.
+  collectgarbage()
+
+  local res, err = misc.memprof.start(filename)
+  -- Should start succesfully.
+  assert(res, err)
+
+  payload()
+
+  res, err = misc.memprof.stop()
+  -- Should stop succesfully.
+  assert(res, err)
+end
+
+generate_output(TMP_BINFILE, default_payload)
+
+local errcode = os.execute(EXECUTABLE .. ' -tm ' .. BAD_PATH)
+test:ok(errcode ~= 0, 'binfile does not exist')
+
+errcode = os.execute(EXECUTABLE .. ' -tm ' .. TMP_BINFILE)
+test:ok(errcode == 0, 'memprof binfile parsing')
+
+os.remove(TMP_BINFILE)
+os.exit(test:check() and 0 or 1)
-- 
2.40.1

Re: [Tarantool-patches] [PATCH luajit v6] memprof: introduce cli flag to run dump parser

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM, after fixing the comments below.

On 05.07.23, Maxim Kokryashkin wrote:
> It is really inconvenient to use a standalone shell script to parse
> memprof dump. That is why this commit introduces a CLI flag for tools
> to the LuaJIT, so now it is possible to parse memprof dump
> as simple as:
> ```
> luajit -tm memprof.bin
> ```
> 
> Closes tarantool/tarantool#5688
> ---
> Changes in v6:
> - Simplified the patch.

It's really nice that we don't use any tricks with the environment now!
Good job!

> 
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
> PR: https://github.com/tarantool/tarantool/pull/8002
> 
>  src/luajit.c                                  | 31 ++++++++++-
>  .../gh-5688-memprof-cli-flag.test.lua         | 55 +++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> 
> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..81c0f8af 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c

<snipped>

> +static int dotoolcmd(lua_State *L, const char *cmd)
> +{
> +  if(strcmp(cmd, "m") == 0) {

Typo: s<if(><if (>

If run with invalid cli flag value there is no error message shown.
| >>> src/luajit -tblabla /tmp/t.lua
| 14:56:56 jobs:0 burii@root:~/reviews/luajit/cli-flags$[1] 

It will be nice to see some test for it.

> +    lua_getglobal(L, "require");
> +    lua_pushliteral(L, "memprof");
> +    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");

May be it is better to say that:
"tools.* modules not installed", since it isn't LuaJIT command.

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

<snipped>

> diff --git a/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> new file mode 100644
> index 00000000..ba0cad68
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> @@ -0,0 +1,55 @@
> +local tap = require('tap')
> +local test = tap.test('gh-5688-cli-for-memprof-parse'):skipcond({
> +  ['Memprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
> +                                               jit.arch ~= 'x64',
> +  ['Memprof is implemented for Linux only'] = jit.os ~= 'Linux',

Side note: I suppose that this is because memprof use symbol table dump
for C functions. But there is no table dumped for OSX. But this isn't a
reason to restrict memprof usage for this platform -- we can still
report C symbols as raw addresses, like it was done before. May you,
please, create a ticket for it (or for dumping C symbols for OSX as well
if you prefer this way :))?

> +-- XXX: The patch is for LuaJIT only, and it doesn't

<snipped>

> -- 
> 2.40.1
> 

-- 
Best regards,
Sergey Kaplun