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

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

[-- Attachment #1: Type: text/plain, Size: 7261 bytes --]


Hi, Sergey!
Thanks for the review!
Fixed all of your comments, except for the one about
`luacheckrc` and _TARANTOOL. I believe, it should be
done in a separate patch.
Here is the diff for other changes:
 
diff --git a/src/luajit.c b/src/luajit.c
index 84472464..b63c92d1 100644
--- a/src/luajit.c
+++ b/src/luajit.c
@@ -79,6 +79,17 @@ static void print_usage(void)
   fflush(stderr);
 }
 
+static void print_tools_usage(void)
+{
+  fputs("usage: ", stderr);
+  fputs(progname, stderr);
+  fputs(" -t<cmd>\n"
+  "Available tools are:\n"
+  "  -m [--leak-only] input  Memprof profile data parser.\n"
+  "  -s input                Sysprof profile data parser.\n", stderr);
+  fflush(stderr);
+}
+
 static void l_message(const char *pname, const char *msg)
 {
   if (pname) { fputs(pname, stderr); fputc(':', stderr); fputc(' ', stderr); }
@@ -387,7 +398,7 @@ static int dotoolcmd(lua_State *L, const char *cmd)
   case 's':
     return runtoolcmd(L, "sysprof");
   default:
-    l_message(progname, "unknown tool command");
+    print_tools_usage();
     break;
   }
   return -1;
@@ -454,8 +465,6 @@ static int collectargs(char **argv, int *flags)
       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;
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 ce99535a..8187c594 100644
--- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
@@ -28,12 +28,11 @@ local TABLE_SIZE = 20
 local SMOKE_CMD_SET = {
   {
     cmd = EXECUTABLE .. ' -t ' .. BAD_PATH,
-    -- luacheck: no global
-    like = _TARANTOOL and '.+unknown tool command' or 'usage:.+',
+    like = '.+Available tools.+',
   },
   {
     cmd = EXECUTABLE .. ' -ta ' .. BAD_PATH,
-    like = '.+unknown tool command',
+    like = '.+Available tools.+',
   },
 }
 
 
 
--
Best regards,
Maxim Kokryashkin
 
  
>Четверг, 7 сентября 2023, 12:41 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Hi, Max
>
>
>On  8/31/23 14 :35, 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 in 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
>
>$ ./build/src/luajit -ts --split sysprof.bin
>luajit-parse-sysprof.lua: ERROR: unrecognized option `--split'. Try
>`--help'.
>
>
>> ```
>>
>> The `luajit-parse-memprof` and `luajit-parse-sysprof` are purged
>> as a result of these changes.
>Typo: s/The/The scripts/
>>
>> Closes tarantool/tarantool#5688
>> ---
><snipped>
>> diff --git a/src/luajit.c b/src/luajit.c
>> index 3a3ec247..f57b7e95 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"
>
>
>When I run "luajit -b" without arguments LuaJIT shows me a usage for "-b":
>
>[0] ~/sources/MRG/tarantool/third_party/luajit$ ./build/src/luajit -b
>Save LuaJIT bytecode: luajit -b[options] input output
>   -l        Only list bytecode.
>   -s        Strip debug info (default).
>   -g        Keep debug info.
>   -n name   Set module name (default: auto-detect from input name).
>   -t type   Set output file type (default: auto-detect from output name).
>   -a arch   Override architecture for object files (default: native).
>   -o os     Override OS for object files (default: native).
>   -e chunk  Use chunk string as input.
>   --        Stop handling options.
>   -         Use stdin as input and/or stdout as output.
>
>File types: c h obj o raw (default)
>
>
>When I run "luajit -t" without arguments LuaJIT shows me the same usage,
>that is not helpful:
>
>
>[0] ~/sources/MRG/tarantool/third_party/luajit$ ./build/src/luajit -t
>usage: ./build/src/luajit [options]... [script [args]...].
>Available options are:
>   -e chunk  Execute string 'chunk'.
>   -l name   Require library 'name'.
>   -b ...    Save or list bytecode.
>   -j cmd    Perform LuaJIT control command.
>   -O[opt]   Control LuaJIT optimizations.
>   -i        Enter interactive mode after executing 'script'.
>   -v        Show version information.
>   -t<cmd>   Execute tool.
>   -E        Ignore environment variables.
>   --        Stop handling options.
>   -         Execute stdin and stop handling options.
>
>Please show a usage with available arguments for "-t":
>
>
>Parse profile dump: luajit -t[options] input
>   -m       parse memprof dump.
>   -s        parse sysprof dump (default).
>
>   --leak-only ...
>
>   --split ...
>
>
>
>> " -E Ignore environment variables.\n"
>> " -- Stop handling options.\n"
>> " - Execute stdin and stop handling options.\n", stderr);
>> @@ -361,6 +362,37 @@ static int dojitcmd(lua_State *L, const char *cmd)
>> return runcmdopt(L, opt ? opt+1 : opt);
>> }
>>
>
><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..4dc4f6a1
>> --- /dev/null
>> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
>> @@ -0,0 +1,127 @@
>> +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',
>
>indentation
>
>
>> + ['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 REDIRECT_OUTPUT = ' 2>&1'
>> +
>> +local TABLE_SIZE = 20
>> +
>> +local SMOKE_CMD_SET = {
>> + {
>> + cmd = EXECUTABLE .. ' -t ' .. BAD_PATH,
>> + -- luacheck: no global
>> + like = _TARANTOOL and '.+unknown tool command' or 'usage:.+',
>
>Let's define this global in .luacheckrc.
>
>I think we will use _TARANTOOL more and this inline suppressions will be
>annoying.
>
>
><no obvious problems, snipped>
 

[-- Attachment #2: Type: text/html, Size: 10029 bytes --]