[Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode
Sergey Bronnikov
sergeyb at tarantool.org
Mon Feb 24 21:05:08 MSK 2025
Hi, Sergey,
changes applied and force-pushed.
Sergey
On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote:
> 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.
Added.
>
>> ---
>> 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 <src/Makefile.dep.original> 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.
Fixed:
--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -180,7 +180,7 @@ static int set_output_path(const char *path, struct
luam_Sysprof_Options *opt) {
}
static int parse_sysprof_opts(lua_State *L, struct
luam_Sysprof_Options *opt,
- const char **err_details) {
+ const char **err_details) {
int n = (int)(L->top - L->base);
if (n == 0) {
opt->mode = LUAM_SYSPROF_DEFAULT;
>
>> + 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.
Fixed:
int n = (int)(L->top - L->base);
+ GCtab *options;
if (n == 0) {
opt->mode = LUAM_SYSPROF_DEFAULT;
opt->interval = SYSPROF_DEFAULT_INTERVAL;
return PROFILE_SUCCESS;
}
- GCtab *options = lj_lib_checktab(L, 1);
+ options = lj_lib_checktab(L, 1);
/* Get profiling mode. */
{
>
> Please, also add the comment that we ignore all other arguments given to
> this function.
Added:
if (n == 0) {
opt->mode = LUAM_SYSPROF_DEFAULT;
opt->interval = SYSPROF_DEFAULT_INTERVAL;
return PROFILE_SUCCESS;
}
- GCtab *options = lj_lib_checktab(L, 1);
+ /* All other arguments given to this function are ignored. */
+ options = lj_lib_checktab(L, 1);
/* Get profiling mode. */
{
>
>>
>> /* Get profiling mode. */
>> {
> <snipped>
>
>> }
>> @@ -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.
Added:
@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct
luam_Sysprof_Options *opt,
int status = 0;
cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
- if (!pathtv)
+ if (!pathtv) {
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 {
path = strVdata(pathtv);
+ }
ctx = lj_mem_new(L, sizeof(*ctx));
ctx->g = G(L);
>
>> 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.
@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct
luam_Sysprof_Options *opt,
int status = 0;
cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path"));
- if (!pathtv)
+ if (!pathtv) {
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 {
path = strVdata(pathtv);
+ }
ctx = lj_mem_new(L, sizeof(*ctx));
ctx->g = G(L);
>
>> 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;
>> }
> <snipped>
>
>> +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.
Removed:
@@ -277,21 +280,16 @@ static int sysprof_error(lua_State *L, int status,
const char *err_details)
switch (status) {
case PROFILE_ERRUSE:
lua_pushnil(L);
- if (err_details) {
+ if (err_details)
lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE),
err_details);
- } else {
+ else
lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
- }
lua_pushinteger(L, EINVAL);
return 3;
>> + 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.
Removed:
#if LJ_HASSYSPROF
case PROFILE_ERRRUN:
lua_pushnil(L);
- if (err_details) {
- lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE),
err_details);
- } else {
- lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
- }
+ lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
lua_pushinteger(L, EINVAL);
return 3;
case PROFILE_ERRIO:
>> + 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.
Yep, irrelevant now.
>
>> + } 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)
> <snipped>
>
>> 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"
Fixed:
--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING, "profiler is not running")
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")
-ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")
#undef ERRDEF
>
>> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
> This errmsg is unused now and can be deleted.
Thanks, fixed:
--- a/src/lj_errmsg.h
+++ b/src/lj_errmsg.h
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING, "profiler is not running")
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")
-ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")
#undef ERRDEF
@@ -127,6 +127,13 @@ test:ok(res == nil and err:match("No such file or
directory"),
"result status and error with bad path")
test:ok(type(errno) == "number", "errno with bad path")
+-- Path is not a string.
+res, err, errno = misc.sysprof.start({ mode = 'C', path = 190 })
+test:ok(res == nil, "result status with path is not a string")
+test:ok(err:match("profiler path should be a string"),
+ "err with path is not a string")
+test:ok(type(errno) == "number", "errno with path is not a string")
+
-- Bad interval.
res, err, errno = misc.sysprof.start{ mode = "C", interval =
BAD_INTERVAL }
test:is(res, nil, "result status and error with bad interval")
>
>> +
>> #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
> <snipped>
>
>> @@ -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.
BAD_INTERVAL is used in two testcases therefore it is declared as constant.
>>
>> local function payload()
>> local function fib(n)
>> @@ -64,11 +66,44 @@ end
> <snipped>
>
>> 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.
Added:
@@ -77,6 +79,9 @@ 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")
+local ok, err_msg = pcall(read_file, SYSPROF_DEFAULT_OUTPUT_FILE)
+test:ok(ok == false and err_msg:match("cannot open a file"),
+ "default output file is empty")
assert(misc.sysprof.stop())
-- Not a table.
>> +
>> +-- Not a table.
>> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
>> +print(res, err, errno)
> Excess debug print.
Removed, thanks!
>
>> +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.
The check is cheap, why not?
>
>> +
>> +-- 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
>
Verbally decided to left these testcases with a comment:
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")
--- All parameters are invalid.
+-- All parameters are invalid. Check parameter validation order
+-- (not strict documented behaviour).
res, err, errno = misc.sysprof.start({
mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })
test:ok(res == nil, "res with all invalid parameters")
@@ -95,7 +100,8 @@ 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.
+-- All parameters are invalid, except the first one. Check
+-- parameter validation order (not strict documented behaviour).
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")
>> +
>> -- Already running.
>> res, err = misc.sysprof.start{ mode = "D" }
>> assert(res, err)
>> @@ -93,11 +128,30 @@test:ok(res == nil anderr: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 anderr: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.
decided to left as is
>
>> +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
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20250224/6b87ecd1/attachment.htm>
More information about the Tarantool-patches
mailing list