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

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

Hi, Maxim!

Thanks for the patch!
Sorry, for such late review!

Please consider my comments below.

On 12.09.21, Maxim Kokryashkin wrote:
> It is really unconvinient to use a standalone shell script to parse

Typo: s/unconvinient/inconvenient/

> memprof dump. That is why this commit intoroduces a CLI flag for tools

Typo: s/intoroduces/introduces/

> to the LuaJIT, so now it is possible to parse memprof dump
> as simple as:
> ```
> luajit -tm memprof.bin
> ```

I don't see any error message in case of unexisted file or anything.

| $ src/luajit -tm /tmp/abracadabra
|

Also, it doesn't work for existing valid files:
| $ src/luajit -tm memprof.bin; echo $?
| 1
|
| $ tools/luajit-parse-memprof memprof.bin
| ALLOCATIONS
| INTERNAL: 948 events    +32167 bytes    -0 bytes
| ...

Am I doing smth wrong?

> 
> Closes tarantool/tarantool#5688
> ---
> GitHub branch:
> https://github.com/tarantool/luajit/tree/fckxorg/gh-5688-cli-for-memprof-parse
> 
> Issue:
> https://github.com/tarantool/tarantool/issues/5688
> 
> Also, I am not sure whether it is ok to do that[1] trick with `argn` or
> not. It seems reasonable to run tools using the `runargs` method, but
> then the `argn` should be adjusted in a such way, that the
> `createargtable` will save parameters for the tool and 'runargs' will
> execute the script itself.
> 
> [1]: https://github.com/tarantool/luajit/blob/8c074ebe7336a27f15484f1bb7eff757fa92c2f4/src/luajit.c#L565
> 
>  CMakeLists.txt         |  9 ++++----
>  src/CMakeLists.txt     |  5 +++++
>  src/lj_tools_conf.h.in |  6 ++++++
>  src/luajit.c           | 49 ++++++++++++++++++++++++++++++++----------
>  tools/CMakeLists.txt   |  2 ++
>  5 files changed, 56 insertions(+), 15 deletions(-)

Should we update original Makefile as well?
Also, it will be nice to add any tests for the new feature.

>  create mode 100644 src/lj_tools_conf.h.in
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 5348e043..74826095 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -250,6 +250,11 @@ endif()

<snipped>

> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 809aac68..d9debccf 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt

<snipped>

> diff --git a/src/lj_tools_conf.h.in b/src/lj_tools_conf.h.in
> new file mode 100644
> index 00000000..366c3ec4
> --- /dev/null
> +++ b/src/lj_tools_conf.h.in

<snipped>

> diff --git a/src/luajit.c b/src/luajit.c
> index 1ca24301..525797a5 100644
> --- a/src/luajit.c
> +++ b/src/luajit.c
> @@ -19,6 +19,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 +74,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,21 +269,17 @@ 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;
>      lua_getglobal(L, "arg");
>      if (lua_istable(L, -1)) {
>        do {
> -	narg++;
> -	lua_rawgeti(L, -narg, narg);
> +	      narg++;
> +	      lua_rawgeti(L, -narg, narg);

I suppose, that this change is unnecessary.

>        } while (!lua_isnil(L, -1));
>        lua_pop(L, 1);
>        lua_remove(L, -narg);
> @@ -290,6 +289,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 +370,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) { 

Typo: trailing whytespace ------^

> +    const int status = call_script(L, PARSER_PATH);
> +    return status == LUA_OK;
> +  }
> +  return -1;
> +}
> +
>  /* Optimization flags. */
>  static int dojitopt(lua_State *L, const char *opt)
>  {
> @@ -398,6 +416,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

Something wrong with indent here.

>  
>  static int collectargs(char **argv, int *flags)
>  {
> @@ -419,14 +438,19 @@ 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 */
>      case 'l':
>        *flags |= FLAGS_OPTION;
>        if (argv[i][2] == '\0') {
> -	i++;
> -	if (argv[i] == NULL) return -1;
> +	      i++;
> +        if (argv[i] == NULL) return -1;

I suppose, that this change is unnecessary.

>        }
>        break;
>      case 'O': break;  /* LuaJIT extension */
> @@ -474,6 +498,9 @@ static int runargs(lua_State *L, char **argv, int argn)
>  	return 1;
>        break;
>        }
> +    case 't' :

Minor: I suggest to add a comment that this is Tarantool's fork
extention.

> +      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 +562,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/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index 61830e44..84129153 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

> -- 
> 2.33.0
> 

-- 
Best regards,
Sergey Kaplun