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 805B4F396C3; Mon, 24 Feb 2025 15:46:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 805B4F396C3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1740401208; bh=CKHgIVwPjUKf1zSZEe1nY0XVgsXHx8rbeeEKd5U9XoA=; 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=PxUYY9k5ez0SLXSP8Hr3O5lR56E1DSzuloKJ0HUNgzaa3qgNgCeLgptLejpMeN+9X 6Q+JfRobYqt4rUIrh58qQ+rZ+jymYFp+f4ZkpndYfpjKToqHNd+2jSjYCKcF/tC9qG AavSlagR5P1WEcfjZHA21Ju+iB6Rysc46PAF3o2Q= Received: from send37.i.mail.ru (send37.i.mail.ru [89.221.237.132]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A2BA3F396C3 for ; Mon, 24 Feb 2025 15:46:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A2BA3F396C3 Received: by exim-smtp-855d9bd6f7-qgjl9 with esmtpa (envelope-from ) id 1tmXrB-000000006uN-20KV; Mon, 24 Feb 2025 15:46:46 +0300 Date: Mon, 24 Feb 2025 15:46:04 +0300 To: Sergey Bronnikov Message-ID: References: <881980705ead69f58c0474d5f49c00603e4c6e72.1740050074.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <881980705ead69f58c0474d5f49c00603e4c6e72.1740050074.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD916C41472748AFA04D4023FF5449FC6F0DDC9B7C52A85C5CD00894C459B0CD1B981E36947506445092EB5D77EF37489D1EEB802E9068861B9EFDB12AB5B95CC7BA43962510F43F2FA X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE782A779A89F7D69B2C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE720512D700D076E85EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BC08E230531AC9C90C9D7013A2B50E3398C6913DCCD035261EF5D174E083ECF91A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A12191B5F2BB8629117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF1580D188F428539ABA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B6A4CD8AA39BF7B65876E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C468D16C903838CAB43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5A131D394E87E19545002B1117B3ED6961ADDC07CE4644A835B6221DB6D7A72AD823CB91A9FED034534781492E4B8EEADA3A806F356AF31D6 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF3965C66B7BCBFA2F6ACE0CA8C02123BDC5BEC476CF9BFDD6407832B4E6261843CC55679CEC9B022BDD1EB5C6CC1B9E9469230B355376089A687BB32EB5B6593AEEC13E71CDE113175F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/CrRuC65GBpQI= X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B596D233DA2D0FA59913DE06ABAFEAF670508F054AD17219882B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode 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, Sergey! Thanks for the fixes! LGTM, after the final polishing, please consider my comments below. On 20.02.25, Sergey Bronnikov wrote: > sysprof has a number of options and with any incorrect option it > returns `false` and error message "profiler misuse". This message > discourages sysprof users and makes using sysprof more > complicated. The patch sets the default profiling mode ("D", that > shows only virtual machine state counters) if it was not passed > and adds details to the error message with possible reasons of > misuse. Please add the following notes. | Also, it fixes the bug when the error isn't raised if the given argument | type is incorrect. | Also, the profiler can start if it has extra arguments given. | They are just ignored now. > --- > src/lib_misc.c | 77 ++++++++++++------- > src/lj_errmsg.h | 5 ++ > .../profilers/misclib-sysprof-lapi.test.lua | 68 ++++++++++++++-- > 3 files changed, 114 insertions(+), 36 deletions(-) > > diff --git a/src/lib_misc.c b/src/lib_misc.c > index 5b7a4b62..1fd06dd1 100644 > --- a/src/lib_misc.c > +++ b/src/lib_misc.c > @@ -14,6 +14,7 @@ > > #include "lj_obj.h" > #include "lj_str.h" > +#include "lj_strfmt.h" Please add the corresponding include (after lj_str.h) in to avoid conflicts when we use this file (for crossbuilds or whatever). > #include "lj_tab.h" > #include "lj_lib.h" > #include "lj_gc.h" > @@ -163,6 +164,7 @@ static int on_stop_cb_default(void *opt, uint8_t *buf) > > /* The default profiling interval equals to 10 ms. */ > #define SYSPROF_DEFAULT_INTERVAL 10 > +#define SYSPROF_DEFAULT_MODE "D" > #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin" > > static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { > @@ -177,21 +179,36 @@ static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { > return PROFILE_SUCCESS; > } > > -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) { > - GCtab *options = lj_lib_checktab(L, idx); > +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > + const char **err_details) { Nit: Please, replace every 8 spaces with one tab. > + int n = (int)(L->top - L->base); > + if (n == 0) { > + opt->mode = LUAM_SYSPROF_DEFAULT; > + opt->interval = SYSPROF_DEFAULT_INTERVAL; > + return PROFILE_SUCCESS; > + } > + > + GCtab *options = lj_lib_checktab(L, 1); Please, declare (not initialize) `options` before `if` branch to avoid -Wdeclaration-after-statement warnings. Please, also add the comment that we ignore all other arguments given to this function. > > /* Get profiling mode. */ > { > } > @@ -214,8 +232,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in > opt->interval = SYSPROF_DEFAULT_INTERVAL; > if (interval && tvisnumber(interval)) { > int32_t signed_interval = numberVint(interval); > - if (signed_interval < 1) > + if (signed_interval < 1) { > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); > return PROFILE_ERRUSE; > + } > opt->interval = signed_interval; > } > } > @@ -230,9 +250,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in > cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path")); > if (!pathtv) Please, add the `{` braces here too. > path = SYSPROF_DEFAULT_OUTPUT; > - else if (!tvisstr(pathtv)) > + else if (!tvisstr(pathtv)) { > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH); > return PROFILE_ERRUSE; > - else > + } else Ditto. > path = strVdata(pathtv); > > ctx = lj_mem_new(L, sizeof(*ctx)); > @@ -251,29 +272,26 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in > return PROFILE_SUCCESS; > } > +static int sysprof_error(lua_State *L, int status, const char *err_details) > { > switch (status) { > case PROFILE_ERRUSE: > lua_pushnil(L); > - lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); > + if (err_details) { Nit: OTOH, there is no need for them here and below. Feel free to ignore. > + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); > + } else { > + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); > + } > lua_pushinteger(L, EINVAL); > return 3; > #if LJ_HASSYSPROF > case PROFILE_ERRRUN: > lua_pushnil(L); > - lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); > + if (err_details) { Side note: `err_deails` is always `NULL` here. I suppose we don't need this branch for now (to avoid the dead code), so these changes may be avoided. > + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/ Irrelevant, after fixing the comment above. > + } else { > + lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); > + } > lua_pushinteger(L, EINVAL); > return 3; > case PROFILE_ERRIO: > @@ -291,15 +309,16 @@ LJLIB_CF(misc_sysprof_start) > diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h > index 19c41f0b..b5c3a275 100644 > --- a/src/lj_errmsg.h > +++ b/src/lj_errmsg.h > @@ -188,6 +188,11 @@ ERRDEF(PROF_ISRUNNING, "profiler is running already") > ERRDEF(PROF_NOTRUNNING, "profiler is not running") > #endif > > +ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'") > +ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1") > +ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist") This should be something like the following: | "profiler path should be a string" > +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters") This errmsg is unused now and can be deleted. > + > #undef ERRDEF > > /* Detecting unused error messages: > diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > index 32fa384c..9915f565 100644 > --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > @@ -24,6 +24,8 @@ local profilename = require("utils").tools.profilename > > local TMP_BINFILE = profilename("sysprofdata.tmp.bin") > local BAD_PATH = profilename("sysprofdata/tmp.bin") > +local BAD_MODE = "A" > +local BAD_INTERVAL = -1 Why do we need this definition for only one type of bad interval? Let's consistently declare both of them like: | local BAD_INTERVAL_M1 = -1 | local BAD_INTERVAL_0 = 0 Or don't declare them at all. I'm OK with both variants. > > local function payload() > local function fib(n) > @@ -64,11 +66,44 @@ end > test:ok(type(errno) == "number", "errno with wrong profiling mode") > > +-- Missed profiling mode. > +res, err, errno = misc.sysprof.start{} > +test:is(res, true, "res with missed profiling mode") > +test:is(err, nil, "no error with missed profiling mode") > +test:is(errno, nil, "no errno with missed profiling mode") > +assert(misc.sysprof.stop()) Minor: It would be nice to check that there is no default file created, i.e., that we actually run the default profiler mode without stack inspection. Feel free to ignore, through. > + > +-- Not a table. > +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE") > +print(res, err, errno) Excess debug print. > +test:is(res, false, "res with not a table") > +test:ok(err:match("table expected, got string"), > + "error with not a table") > +test:is(errno, nil, "errno with not a table") There is no need to check or declare the `errno` variable. It's always `nil` for the case when the error is raised. > + > +-- All parameters are invalid. > +res, err, errno = misc.sysprof.start({ > + mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL }) > +test:ok(res == nil, "res with all invalid parameters") > +test:ok(err:match("profiler misuse: profiler mode must be 'D', 'L' or 'C'"), > + "error with all invalid parameters") > +test:ok(type(errno) == "number", "errno with all invalid parameters") > + > +-- All parameters are invalid, except the first one. > +res, err, errno = misc.sysprof.start({ > + mode = "C", path = BAD_PATH, interval = BAD_INTERVAL }) > +test:ok(res == nil, "res with all invalid parameters except the first one") > +test:ok(err:match("profiler misuse: profiler interval must be greater than 1"), > + "error with all invalid parameters except the first one") > +test:ok(type(errno) == "number", > + "errno with all invalid parameters except the first one") This is not exactly what I mean by invalid/partially invalid parameters (sorry, for the bad wording): I mean to add tests that cover the following calls: | pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error | -- (expected table got nil) | misc.sysprof.start({}, '', '') -- should started with the default options > + > -- Already running. > res, err = misc.sysprof.start{ mode = "D" } > assert(res, err) > @@ -93,11 +128,30 @@ test:ok(res == nil and err:match("No such file or directory"), > test:ok(type(errno) == "number", "errno with bad path") > > -- Bad interval. > -res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 } > -test:ok(res == nil and err:match("profiler misuse"), > - "result status and error with bad interval") > +res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL } Side note: See my comments about this constant above. > +test:is(res, nil, "result status and error with bad interval") > +test:ok(err:match("profiler interval must be greater than 1"), > + "error with bad interval") > test:ok(type(errno) == "number", "errno with bad interval") > > +-- Bad interval (0). > +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 } > +test:ok(res == nil, "res with bad interval 0") > +test:ok(err:match("profiler interval must be greater than 1"), > + "error with bad interval 0") > +test:ok(type(errno) == "number", "errno with bad interval 0") > + > +-- Good interval (1). > +res, err, errno = misc.sysprof.start{ > + mode = "C", > + interval = 1, > + path = "/dev/null", > +} > +test:is(res, true, "res with good interval 1") > +test:is(err, nil, "no error with good interval 1") > +test:is(errno, nil, "no errno with good interval 1") > +misc.sysprof.stop() > + > -- DEFAULT MODE > > if not pcall(generate_output, { mode = "D", interval = 100 }) then > -- > 2.43.0 > -- Best regards, Sergey Kaplun