From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 081C863EB2; Tue, 29 Nov 2022 16:22:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 081C863EB2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1669728121; bh=ut5WrhMnK6OpHTK7j2RZ/a1Sz6aKq82tYshDvVYg8LI=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=T+nyjlGVAyRI+YlnF/cIFwTPILFwCngcbehaSfWm+W1FNtJWSJnnc/JKc6i3HuQll HW29UZ6Arpg99g5AhjT8S/lqdzyYXZ+mfA1NWhm2lHYqpfZtD2zbt5mlet2ZLHrRO+ djxCVUK9ogmX7oUMV8Tc7l2ux0pXlLt83g8KZIIU= Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 866B563EB2 for ; Tue, 29 Nov 2022 16:21:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 866B563EB2 Received: by smtp57.i.mail.ru with esmtpa (envelope-from ) id 1p00Yg-0001Yw-DH; Tue, 29 Nov 2022 16:21:59 +0300 Date: Tue, 29 Nov 2022 16:18:47 +0300 To: Maxim Kokryashkin Message-ID: References: <20210912145118.392561-1-m.kokryashkin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210912145118.392561-1-m.kokryashkin@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F70D9A593BA47E2AA0257E11E22CF08A8D88289CBD1FAF34182A05F5380850401DCA6DB706552823F60C0B3BB830C81E467A6593A78D44C4096DE74FE7527A05 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE77E216A0E97507353EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637BBB1CE3B4DF50705EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B6F1F7B995052D5CEFF0436154D6BC4304D1386B6C5801732CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637EAAFF0EA65433293389733CBF5DBD5E9B5C8C57E37DE458BD9DD9810294C998ED8FC6C240DEA76428AA50765F7900637C17EAD96B13C99AED81D268191BDAD3DBD4B6F7A4D31EC0BEA7A3FFF5B025636D81D268191BDAD3D78DA827A17800CE76CEF212D22A2C45DEC76A7562686271ED91E3A1F190DE8FD2E808ACE2090B5E14AD6D5ED66289B5259CC434672EE63711DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3A0CC8883A33FD9CD35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34505665BFD4C7070531918FF103B5858B98350512FAE5FC094FCF8EE4023E0600505C2E905DF717EA1D7E09C32AA3244C1A251AA92018E931C15E96D5A6F38EB13A92A9747B6CC886927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLCIl0w9hXVzK36kJ/pGG5w== X-Mailru-Sender: 87D5297B137A96FE2CF18BD2B17679283344AB36AF4FC56ED608F6F4DD2C313297F2A1272DDCC651525762887713E5F1475755348978188EF9D3679FA3DE6E791CC59163FFD68303B0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v2] memprof: introduce cli flag to run dump parser X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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() > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 809aac68..d9debccf 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > 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 > 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 > #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 > -- > 2.33.0 > -- Best regards, Sergey Kaplun