Re: [Tarantool-patches] [PATCH] luajit: proxy -j and -b flags

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

Hi, Maxim!

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

On 12.08.21, Maxim Kokryashkin wrote:
> There are two flags in the LuaJIT useful for debugging purposes: `-j`
> and `-b`. However, if you want to check the same Lua code from the
> Tarantool, you will need to make some adjustments in the script itself,
> as those flags are not present in the Tarantool's CLI.
> 
> This patch introduces those flags to the Tarantool, so debugging is
> much more convenient now. Flags are working the same as they do in
> LuaJIT.
> 
> Closes tarantool/tarantool#5541

Minor: As far as it is tarantool repo there is no need to use the full
path. #5541 is enough.

> ---
> Github branch: 
> https://github.com/tarantool/tarantool/tree/fckxorg/gh-5541-proxy-luajit-flags
> 
> Issue:
> https://github.com/tarantool/tarantool/issues/5541
> 
>  src/lua/init.c     | 160 +++++++++++++++++++++++++++++++++++++++++++++
>  src/lua/init.h     |   3 +
>  src/main.cc        | 123 +++++++++++++++++++++-------------
>  third_party/luajit |   2 +-

I don't get from the commit message why do we need to bump LuaJIT
version here (I strongly doubt that we need it)?

>  4 files changed, 241 insertions(+), 47 deletions(-)

Sorry, the patch is not well formated, so it is hard to read the <src/main.cc>
part.

Some general thoughts:
Please rewrite your code considering our code style guide [1][2].
This means:
1) In C use tabs, not whitespaces. Tab width should be 8 symbols. In Lua
   use whitespaces. Indentation step is 4 whitespaces;
2) In C comments out of functions and inside of functions should be
   different in how they are started. Everything else is wrong. Below are
   correct examples. `/**` comes for documentation comments, `/*` for local
   not documented comments. However the difference is vague already, so the
   rule is simple - out of function = `/**`, inside = `/*`.
3) Remove trailing whitespaces.
4) Use separate line for comments.

Also, it would be nice to see some tests for new functionality (even the dummy
one).

Please fix the following comments and resend patch in v2.

> 
> diff --git a/src/lua/init.c b/src/lua/init.c
> index f9738025d..16c8bc4b5 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -313,6 +313,127 @@ skip:		base = (base == -1 ? 10 : base);
>  
>  /* }}} */
>  
> +static void l_message(const char *msg)
> +{
> +  fputs(msg, stderr); fputc('\n', stderr);

Minor: Use separate line here.

XXX: Also, I don't sure about stderr usage:
For example, Tarantool report error in stdout, when the script name is
invalid.

> +  fflush(stderr);
> +}
> +
> +static int report(lua_State *L, int status)
> +{
> +  if (status && !lua_isnil(L, -1)) {
> +    const char *msg = lua_tostring(L, -1);
> +    if (msg == NULL) msg = "(error object is not a string)";

Minor: use separate line here.
Feel free to ignore.

> +    l_message(msg);
> +    lua_pop(L, 1);
> +  }
> +  return status;
> +}
> +
> +/* Load add-on module. */

Please, describe values on Lua stack before and after this call.
It should be mentioned for each function, which works with lua_State.

> +static int loadjitmodule(lua_State* L)
> +{
> +  lua_getglobal(L, "require");
> +  lua_pushliteral(L, "jit.");
> +  lua_pushvalue(L, -3);
> +  lua_concat(L, 2);
> +  if (lua_pcall(L, 1, 1, 0)) {
> +    const char *msg = lua_tostring(L, -1);
> +    if (msg && !strncmp(msg, "module ", 7))
> +      goto nomodule;
> +    return report(L, 1);
> +  }
> +  lua_getfield(L, -1, "start");
> +  if (lua_isnil(L, -1)) {
> +  nomodule:
> +    l_message("unknown luaJIT command or jit.* modules not installed");

Side note: I've thought it should be LuaJIT here, but it's the same in
the LuaJIT sources.

> +    return 1;
> +  }
> +  lua_remove(L, -2);  /* Drop module table. */
> +  return 0;
> +}
> +
> +int dobytecode(va_list ap)

Why can't we call this function on tarantool_L state, as it is done for -j
flag?

> +{
> +	struct lua_State *L = va_arg(ap, struct lua_State *);
> +  char **argv = va_arg(ap, char **);
> +	struct diag *diag = va_arg(ap, struct diag *);
> +
> +  int narg = 0;
> +  bool aux_loop_is_run = false;
> +
> +  lua_pushliteral(L, "bcsave");
> +  if (loadjitmodule(L))
> +    goto error;
> +  if (argv[0][2]) {
> +    narg++;
> +    argv[0][1] = '-';
> +    lua_pushstring(L, argv[0]+1);

Typo: s/argv[0]+1/argv[0] + 1/

> +  }
> +
> +	fiber_sleep(0.0);

Why do we need fiber_sleep here?
Please, drop a comment.

> +	aux_loop_is_run = true;
> +  for (argv++; *argv != NULL; narg++, argv++)
> +    lua_pushstring(L, *argv);
> +  int res = lua_pcall(L, narg, 0, 0);
> +  if(res)

Typo: s/if(/if (/
Minor: use LUA_OK, to check result of lua_pcall.

> +    goto error;
> +end:
> +	if (!aux_loop_is_run)
> +		fiber_sleep(0.0);

Why do we need fiber_sleep here?
Please, drop a comment.

> +	ev_break(loop(), EVBREAK_ALL);
> +	return 0;
> +
> +error:
> +	diag_move(diag_get(), diag);
> +	goto end;
> +}
> +
> +/* Run command with options. */
> +static int runcmdopt(lua_State *L, const char *opt)
> +{
> +  int narg = 0;
> +  if (opt && *opt) {
> +    for (;;) {  /* Split arguments. */
> +      const char *p = strchr(opt, ',');
> +      narg++;
> +      if (!p) break;
> +      if (p == opt)
> +	lua_pushnil(L);
> +      else
> +	lua_pushlstring(L, opt, (size_t)(p - opt));
> +      opt = p + 1;
> +    }
> +    if (*opt)
> +      lua_pushstring(L, opt);
> +    else
> +      lua_pushnil(L);
> +  }
> +  return report(L, lua_pcall(L, narg, 0, 0));
> +}
> +
> +/* JIT engine control command: try jit library first or load add-on module. */
> +int dojitcmd(const char *cmd)
> +{
> +  const char *opt = strchr(cmd, '=');
> +  lua_pushlstring(tarantool_L, cmd, opt ? (size_t)(opt - cmd) : strlen(cmd));
> +  lua_getfield(tarantool_L, LUA_REGISTRYINDEX, "_LOADED");
> +  lua_getfield(tarantool_L, -1, "jit");  /* Get jit.* module table. */
> +  lua_remove(tarantool_L, -2);
> +  lua_pushvalue(tarantool_L, -2);
> +  lua_gettable(tarantool_L, -2);  /* Lookup library function. */
> +  if (!lua_isfunction(tarantool_L, -1)) {
> +    lua_pop(tarantool_L, 2);  /* Drop non-function and jit.* table, keep module name. */
> +    if (loadjitmodule(tarantool_L))
> +      return 1;
> +  } else {
> +    lua_remove(tarantool_L, -2);  /* Drop jit.* table. */
> +  }
> +  lua_remove(tarantool_L, -2);  /* Drop module name. */
> +  return runcmdopt(tarantool_L, opt ? opt+1 : opt);

Typo: s/opt+1/opt + 1/

> +}
> +
> +

Typo: extra empty line.

>  /**
>   * Original LuaJIT/Lua logic: <luajit/src/lib_package.c - function setpath>
>   *
> @@ -608,6 +729,10 @@ run_script_f(va_list ap)
>  			lua_setglobal(L, optv[i + 1]);
>  			lua_settop(L, 0);
>  			break;
> +    case 'j':
> +      if (dojitcmd(optv[i + 1]) != 0)
> +        goto error;
> +      break;
>  		case 'e':
>  			/*
>  			 * Execute chunk
> @@ -747,6 +872,39 @@ tarantool_lua_run_script(char *path, bool interactive,
>  	return diag_is_empty(diag_get()) ? 0 : -1;
>  }
>  
> +int
> +tarantool_lua_dump_bytecode(char **argv) {
> +	script_fiber = fiber_new("bcsave", dobytecode);

IINM, bcsave module don't run a user's code, just precompile it to the
bytecode. Do we need a separate fiber for it?

> +	if (script_fiber == NULL)
> +		panic("%s", diag_last_error(diag_get())->errmsg);
> +	script_fiber->storage.lua.stack = tarantool_L;
> +	/*
> +	 * Create a new diag on the stack. Don't pass fiber's diag, because it
> +	 * might be overwritten by libev callbacks invoked in the scheduler
> +	 * fiber (which is this), and therefore can't be used as a sign of fail
> +	 * in the script itself.
> +	 */
> +	struct diag bc_diag;
> +	diag_create(&bc_diag);
> +	fiber_start(script_fiber, tarantool_L, argv, &bc_diag);
> +	/*
> +	 * Run an auxiliary event loop to re-schedule run_script fiber.
> +	 * When this fiber finishes, it will call ev_break to stop the loop.
> +	 */
> +	if (start_loop)
> +		ev_run(loop(), 0);
> +	/* The fiber running the startup script has ended. */
> +	script_fiber = NULL;
> +	diag_move(&bc_diag, diag_get());
> +	diag_destroy(&bc_diag);
> +	/*
> +	 * Result can't be obtained via fiber_join - script fiber
> +	 * never dies if os.exit() was called. This is why diag
> +	 * is checked explicitly.
> +	 */
> +	return diag_is_empty(diag_get()) ? 0 : -1; 
> +}
> +
>  void
>  tarantool_lua_free()
>  {
> @@ -783,3 +941,5 @@ tarantool_lua_free()
>  	}
>  #endif
>  }
> +
> +

Looks like these changes are unnecessary.

> diff --git a/src/lua/init.h b/src/lua/init.h
> index 7fc0b1a31..4d049ecc8 100644
> --- a/src/lua/init.h
> +++ b/src/lua/init.h
> @@ -74,6 +74,9 @@ int
>  tarantool_lua_run_script(char *path, bool force_interactive,
>  			 int optc, const char **optv,
>  			 int argc, char **argv);
> +int
> +tarantool_lua_dump_bytecode(char **argv);
> +int dojitcmd(const char *cmd);

Why this function should be declared as public for this module if it is used
only inside it?

>  
>  extern char *history;
>  
> diff --git a/src/main.cc b/src/main.cc
> index de082c17f..bd0314852 100644
> --- a/src/main.cc
> +++ b/src/main.cc

<snipped>

>  }
>  
> +enum {
> +  ARG_OK = 1,
> +  ARG_LJ = 2
> +};
> +
> +enum {
> +  O_INTERACTIVE = 1,
> +  O_BYTECODE = 2

I suppose it will be better to declare this flags like the following:

| #define OPT_INTERACTIVE 0x1
| #define OPT_BYTECODE    0x2

To hightlight that they are flags to be set.

> +};
> +
> +static int collect_tarantool_arg(int ch, uint32_t *opt_mask, int argc, char **argv, int *optc, const char ***optv) {

Why do we need a new function with 6 arguments?

> +  switch (ch) {
> +		case 'V':
> +		case 'v':
> +			print_version();
> +			return 0;
> +		case 'h':
> +			print_help(basename(argv[0]));
> +			return 0;
> +		case 'i':
> +			/* Force interactive mode */
> +      *opt_mask |= O_INTERACTIVE;
> +			break;
> +    case 'b':
> +      *opt_mask |= O_BYTECODE;
> +      return ARG_LJ; 
> +    case 'j':
> +    case 'l':
> +		case 'e':
> +			/* Save Lua interepter options to optv as is */
> +			if (*optc == 0) {
> +				*optv = (const char **) calloc(argc,
> +							      sizeof((*optv)[0]));
> +				if (*optv == NULL)
> +					panic_syserror("No enough memory for arguments");
> +			}
> +      if(ch == 'l') (*optv)[(*optc)++] = "-l";
> +      else if(ch == 'j') (*optv)[(*optc)++] = "-j";
> +      else (*optv)[(*optc)++] = "-e";
> +			(*optv)[(*optc)++] = optarg;
> +			break;
> +		default:
> +			/* "invalid option" is printed by getopt */
> +			return EX_USAGE;
> +	}
> +  return ARG_OK;
> +}
> +
>  extern "C" void **
>  export_syms(void);
>  
> @@ -598,7 +648,7 @@ main(int argc, char **argv)
>  	fpconv_check();
>  
>  	/* Enter interactive mode after executing 'script' */
> -	bool interactive = false;
> +  uint32_t opt_mask = 0;
>  	/* Lua interpeter options, e.g. -e and -l */
>  	int optc = 0;
>  	const char **optv = NULL;
> @@ -609,58 +659,35 @@ main(int argc, char **argv)
>  		{"version", no_argument, 0, 'v'},
>  		{NULL, 0, 0, 0},
>  	};
> -	static const char *opts = "+hVvie:l:";
> +	static const char *opts = "+hVvbij:e:l:";
>  
>  	int ch;
>  	while ((ch = getopt_long(argc, argv, opts, longopts, NULL)) != -1) {
> -		switch (ch) {
> -		case 'V':
> -		case 'v':
> -			print_version();
> -			return 0;
> -		case 'h':
> -			print_help(basename(argv[0]));
> -			return 0;
> -		case 'i':
> -			/* Force interactive mode */
> -			interactive = true;
> -			break;
> -		case 'l':
> -		case 'e':
> -			/* Save Lua interepter options to optv as is */
> -			if (optc == 0) {
> -				optv = (const char **) calloc(argc,
> -							      sizeof(optv[0]));
> -				if (optv == NULL)
> -					panic_syserror("No enough memory for arguments");
> -			}
> -			optv[optc++] = ch == 'l' ? "-l" : "-e";
> -			optv[optc++] = optarg;
> -			break;
> -		default:
> -			/* "invalid option" is printed by getopt */
> -			return EX_USAGE;
> -		}
> +    int res = collect_tarantool_arg(ch, &opt_mask, argc, argv, &optc, &optv);
> +    if(res == 0 || res == EX_USAGE) return res;
> +    if(res == ARG_LJ) break;
>  	}
>  
>  	/* Shift arguments */
>  	argc = 1 + (argc - optind);
>  	for (int i = 1; i < argc; i++)
>  		argv[i] = argv[optind + i - 1];
> -
> -	if (argc > 1 && strcmp(argv[1], "-") && access(argv[1], R_OK) != 0) {
> -		/*
> -		 * Somebody made a mistake in the file
> -		 * name. Be nice: open the file to set
> -		 * errno.
> -		 */
> -		int fd = open(argv[1], O_RDONLY);
> -		int save_errno = errno;
> -		if (fd >= 0)
> -			close(fd);
> -		printf("Can't open script %s: %s\n", argv[1], strerror(save_errno));
> -		return save_errno;
> -	}
> +  
> +  if(!(opt_mask & O_BYTECODE)) {
> +    if (argc > 1 && strcmp(argv[1], "-") && access(argv[1], R_OK) != 0) {
> +      /*
> +       * Somebody made a mistake in the file
> +       * name. Be nice: open the file to set
> +       * errno.
> +       */
> +      int fd = open(argv[1], O_RDONLY);
> +      int save_errno = errno;
> +      if (fd >= 0)
> +        close(fd);
> +      printf("Can't open script %s: %s\n", argv[1], strerror(save_errno));
> +      return save_errno;
> +    }
> +  }
>  
>  	argv = title_init(argc, argv);
>  	/*
> @@ -751,13 +778,17 @@ main(int argc, char **argv)
>  			panic("%s", "can't init event loop");
>  
>  		int events = ev_activecnt(loop());
> -		/*
> +
> +    if(opt_mask & O_BYTECODE) {
> +      return tarantool_lua_dump_bytecode(argv);
> +    }
> +    /*
>  		 * Load user init script.  The script should have access
>  		 * to Tarantool Lua API (box.cfg, box.fiber, etc...) that
>  		 * is why script must run only after the server was fully
>  		 * initialized.
>  		 */
> -		if (tarantool_lua_run_script(script, interactive, optc, optv,
> +		if (tarantool_lua_run_script(script, opt_mask & O_INTERACTIVE, optc, optv,
>  					     main_argc, main_argv) != 0)
>  			diag_raise();
>  		/*
> diff --git a/third_party/luajit b/third_party/luajit
> index e7f701639..436898f4e 160000
> --- a/third_party/luajit
> +++ b/third_party/luajit
> @@ -1 +1 @@
> -Subproject commit e7f701639cc6900edeef4cec22776ea04e33895d
> +Subproject commit 436898f4eb69902c1adf82a302717f47b48ffe2c
> -- 
> 2.32.0
> 

[1]: https://www.tarantool.io/en/doc/latest/dev_guide/c_style_guide/
[2]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure

-- 
Best regards,
Sergey Kaplun