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

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

Hi!

Thanks for the patch!
I got couple of comments below.

Thanks,
Sergos

> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..a655410f 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
> @@ -9,6 +9,7 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <sys/errno.h>
> 

Looks superfluous. The only reason the `update_env_var` can return -1 is incorrect name,
which in part because the env is too big. The error is always leads to the `print_usage`,
so I would drop the errno completely.

> #define luajit_c
> 
> @@ -19,6 +20,8 @@
> 
> #include "lj_arch.h"
> 
> +#include "lj_tools_conf.h"
> +
> #if LJ_TARGET_POSIX
> #include <unistd.h>
> #define lua_stdin_is_tty()	isatty(0)
> @@ -72,6 +75,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);
> @@ -266,13 +270,9 @@ static void dotty(lua_State *L)
>   progname = oldprogname;
> }
> 
> -static int handle_script(lua_State *L, char **argx)
> +static int call_script(lua_State *L, const char *fname)
> {
> -  int status;
> -  const char *fname = argx[0];
> -  if (strcmp(fname, "-") == 0 && strcmp(argx[-1], "--") != 0)
> -    fname = NULL;  /* stdin */
> -  status = luaL_loadfile(L, fname);
> +  int status = luaL_loadfile(L, fname);
>   if (status == LUA_OK) {
>     /* Fetch args from arg table. LUA_INIT or -e might have changed them. */
>     int narg = 0;
> @@ -290,6 +290,16 @@ static int handle_script(lua_State *L, char **argx)
>     }
>     status = docall(L, narg, 0);
>   }
> +  return status;
> +}
> +
> +static int handle_script(lua_State *L, char **argx)
> +{
> +  int status;
> +  const char *fname = argx[0];
> +  if (strcmp(fname, "-") == 0 && strcmp(argx[-1], "--") != 0)
> +    fname = NULL;  /* stdin */
> +  status = call_script(L, fname);
>   return report(L, status);
> }
> 
> @@ -361,6 +371,15 @@ 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) {
> +    const int status = call_script(L, PARSER_PATH);
> +    return report(L, status);
> +  }
> +  return -1;
> +}
> +
> /* Optimization flags. */
> static int dojitopt(lua_State *L, const char *opt)
> {
> @@ -390,6 +409,38 @@ static int dobytecode(lua_State *L, char **argv)
>   return -1;
> }
> 
> +/*
> +** On most Linux distros, it is the default value for the
> +** maximum length of a string passed to `execve`.
> +** However, there is no common value for other OSes, so
> +** the size of 32 default memory pages is adopted.
> +*/
> +#define MAX_ENV_VAR 32 * 4096
> +
> +static int update_env_var(const char *name, const char *value)
> +{
> +  char env_buf[MAX_ENV_VAR] = "";
> +  const char *env = getenv(name);
> +  /*
> +  ** The `+ 1` is added here to handle case where `env` is
> +  ** empty and `value` is too long without any additional
> +  ** `if` statements.
> +  */
> +  size_t env_len = strnlen(env, MAX_ENV_VAR + 1);
> +  size_t value_len = strnlen(value, MAX_ENV_VAR + 1);
> +  if (value_len + env_len > MAX_ENV_VAR) {
> +    errno = ENOMEM;
> +    return -1;
> +  }
> +
> +  if (env == NULL) {
> +    return setenv(name, value, 0);
> +  } else {
> +    strncpy(env_buf, env, env_len);
> +    return setenv(name, strncat(env_buf, value, MAX_ENV_VAR - value_len), 1);
> +  }
> +}
> +
> /* check that argument has no extra characters at the end */
> #define notail(x)	{if ((x)[2] != '\0') return -1;}
> 
> @@ -398,10 +449,12 @@ 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)
> {
>   int i;
> +  int result;
>   for (i = 1; argv[i] != NULL; i++) {
>     if (argv[i][0] != '-')  /* Not an option? */
>       return i;
> @@ -419,6 +472,14 @@ 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;
> +      result = update_env_var("LUA_PATH", TOOLS_PATH);
> +      if (result != 0)
> +        return result;
> +      return i + 1;
>     case 'e':
>       *flags |= FLAGS_EXEC;
>     case 'j':  /* LuaJIT extension */
> @@ -474,6 +535,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 +600,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..ad7bf732
> --- /dev/null
> +++ b/test/tarantool-tests/gh-5688-memprof-cli-flag.test.lua
> @@ -0,0 +1,56 @@
> +local utils = require('utils')
> +
> +-- XXX: The patch is for LuaJIT only, and it doesn't
> +-- work on Tarantool.
> +utils.skipcond(
> +-- luacheck: no global
> +  (jit.arch ~= 'x86' and jit.arch ~= 'x64') or _TARANTOOL,
> +  jit.arch..' architecture is NIY for memprof’

Which will mislead for x64 under Tarantool - perhaps, mention ‘memprof tools’?

> +)
> +
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5688-memprof-cli-flag')
> +test:plan(2)
> +
> +jit.off()
> +jit.flush()
> +
> +local table_new = require 'table.new'
> +
> +local TMP_BINFILE = utils.profilename('memprofdata.tmp.bin')
> +local BAD_PATH = utils.profilename('bad-path-tmp.bin')
> +local EXECUTABLE = utils.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)