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 07F2F4195A3; Tue, 2 May 2023 19:21:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 07F2F4195A3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1683044516; bh=POsWsCApWFCMEAEmVi6xqyLBCN/c3gZnQFEppK5Prvo=; h=In-Reply-To:Date:References:To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=hfZYGt/WtnzubCCXoQOjx38fDcbKjVIO4rHjNsAWyQoP6ySbENcpJc0y/SgP/TgDn t3bxmkKnSUu9GeC10p273fAcDEKqk6BrQsDVcr6WANzX/eOAVrlwF7CXH6ZbbKwose ZSt/+TUdH4QXEwirzV0MFGuNEd7T8VK+0qvM0t20= Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [95.163.41.77]) (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 A8F4270370 for ; Tue, 2 May 2023 19:21:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A8F4270370 Received: by smtp36.i.mail.ru with esmtpa (envelope-from ) id 1ptslF-00A1RA-Lo; Tue, 02 May 2023 19:21:54 +0300 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\)) In-Reply-To: <20230209121741.66299-1-max.kokryashkin@gmail.com> Date: Tue, 2 May 2023 19:21:43 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <688A88B7-42C5-4422-90D9-49266637A18B@tarantool.org> References: <20230209121741.66299-1-max.kokryashkin@gmail.com> To: Maksim Kokryashkin X-Mailer: Apple Mail (2.3731.500.231) X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9F572AEF33F1BC585BEE51F2F47BB93C86123CED0B985D7B0182A05F5380850404FA000F12F5E6EFD7231D88F55D99F30C3C9CDD7F06AEABC6AABE8150079D201 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A3DED2DACB82E709C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE74ABD1465354149F7EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B6F1F7B995052D5CEC22951D036BE9617E5CC01F9FDF07FFBCC7F00164DA146DAFE8445B8C89999728AA50765F7900637E8411255775BB20C389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC8E6BDB36F057AC83CF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C64A52DF3AE21010F6E0066C2D8992A164AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3705B0E1A13332C17BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE78D1F1305E8F8D506731C566533BA786AA5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A5D2C695FA7C55C25F67D5C3F7E9FD4B2E5D95E169A3A1A420F87CCE6106E1FC07E67D4AC08A07B9B062B3BD3CC35DA588CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF6D867A33384AA745839BAFA95ED43494B1D84E66EB0A04CD4E90E95289B64698EBD4E4314DA78065675ADA185E77B7F5C90E3AF6A9609F45AB68676377D2B27EF4E8A8FB6BF8EBF5 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUsZxtR84vlfheSxvNpN4zA== X-Mailru-Sender: 5AA3D5B9D8C486466A1CB9683971E69C8B2072F21AEA611FE25DD73F756E977C577B3D43C2D23E6F19381EE24192DF5555834048F03EF5D4C9A814A92B2E3B1BA4250FC3964EA4964198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit v4] 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: sergos via Tarantool-patches Reply-To: sergos Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 > #include > #include > +#include >=20 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 >=20 > @@ -19,6 +20,8 @@ >=20 > #include "lj_arch.h" >=20 > +#include "lj_tools_conf.h" > + > #if LJ_TARGET_POSIX > #include > #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 =3D oldprogname; > } >=20 > -static int handle_script(lua_State *L, char **argx) > +static int call_script(lua_State *L, const char *fname) > { > - int status; > - const char *fname =3D argx[0]; > - if (strcmp(fname, "-") =3D=3D 0 && strcmp(argx[-1], "--") !=3D 0) > - fname =3D NULL; /* stdin */ > - status =3D luaL_loadfile(L, fname); > + int status =3D luaL_loadfile(L, fname); > if (status =3D=3D LUA_OK) { > /* Fetch args from arg table. LUA_INIT or -e might have changed = them. */ > int narg =3D 0; > @@ -290,6 +290,16 @@ static int handle_script(lua_State *L, char = **argx) > } > status =3D docall(L, narg, 0); > } > + return status; > +} > + > +static int handle_script(lua_State *L, char **argx) > +{ > + int status; > + const char *fname =3D argx[0]; > + if (strcmp(fname, "-") =3D=3D 0 && strcmp(argx[-1], "--") !=3D 0) > + fname =3D NULL; /* stdin */ > + status =3D call_script(L, fname); > return report(L, status); > } >=20 > @@ -361,6 +371,15 @@ static int dojitcmd(lua_State *L, const char = *cmd) > return runcmdopt(L, opt ? opt+1 : opt); > } >=20 > +static int dotoolcmd(lua_State *L, const char *cmd) > +{ > + if(strcmp(cmd, "m") =3D=3D 0) { > + const int status =3D 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; > } >=20 > +/* > +** 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] =3D ""; > + const char *env =3D 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 =3D strnlen(env, MAX_ENV_VAR + 1); > + size_t value_len =3D strnlen(value, MAX_ENV_VAR + 1); > + if (value_len + env_len > MAX_ENV_VAR) { > + errno =3D ENOMEM; > + return -1; > + } > + > + if (env =3D=3D 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] !=3D '\0') return -1;} >=20 > @@ -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 >=20 > static int collectargs(char **argv, int *flags) > { > int i; > + int result; > for (i =3D 1; argv[i] !=3D NULL; i++) { > if (argv[i][0] !=3D '-') /* Not an option? */ > return i; > @@ -419,6 +472,14 @@ static int collectargs(char **argv, int *flags) > notail(argv[i]); > *flags |=3D FLAGS_VERSION; > break; > + case 't': > + *flags |=3D FLAGS_TOOL; > + if (argv[i][2] =3D=3D '\0') return -1; > + if (argv[i + 1] =3D=3D NULL) return -1; > + result =3D update_env_var("LUA_PATH", TOOLS_PATH); > + if (result !=3D 0) > + return result; > + return i + 1; > case 'e': > *flags |=3D 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 =3D argv[i] + 2; > + return dotoolcmd(L, cmd) =3D=3D 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); >=20 > - createargtable(L, argv, s->argc, argn); > + createargtable(L, argv, s->argc, (flags & FLAGS_TOOL) ? argn - 1 : = argn); >=20 > if (!(flags & FLAGS_NOENV)) { > s->status =3D 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 =3D require('utils') > + > +-- XXX: The patch is for LuaJIT only, and it doesn't > +-- work on Tarantool. > +utils.skipcond( > +-- luacheck: no global > + (jit.arch ~=3D 'x86' and jit.arch ~=3D 'x64') or _TARANTOOL, > + jit.arch..' architecture is NIY for memprof=E2=80=99 Which will mislead for x64 under Tarantool - perhaps, mention =E2=80=98mem= prof tools=E2=80=99? > +) > + > +local tap =3D require('tap') > + > +local test =3D tap.test('gh-5688-memprof-cli-flag') > +test:plan(2) > + > +jit.off() > +jit.flush() > + > +local table_new =3D require 'table.new' > + > +local TMP_BINFILE =3D utils.profilename('memprofdata.tmp.bin') > +local BAD_PATH =3D utils.profilename('bad-path-tmp.bin') > +local EXECUTABLE =3D utils.luacmd(arg) > +local TABLE_SIZE =3D 20 > + > +local function default_payload() > + local _ =3D table_new(TABLE_SIZE, 0) > + _ =3D nil > + collectgarbage() > +end > + > +local function generate_output(filename, payload) > + -- Clean up all garbage to avoid pollution of free. > + collectgarbage() > + > + local res, err =3D misc.memprof.start(filename) > + -- Should start succesfully. > + assert(res, err) > + > + payload() > + > + res, err =3D misc.memprof.stop() > + -- Should stop succesfully. > + assert(res, err) > +end > + > +generate_output(TMP_BINFILE, default_payload) > + > +local errcode =3D os.execute(EXECUTABLE .. ' -tm ' .. BAD_PATH) > +test:ok(errcode ~=3D 0, 'binfile does not exist') > + > +errcode =3D os.execute(EXECUTABLE .. ' -tm ' .. TMP_BINFILE) > +test:ok(errcode =3D=3D 0, 'memprof binfile parsing') > + > +os.remove(TMP_BINFILE) > +os.exit(test:check() and 0 or 1)