Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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 <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.

> +  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. */
>    {

<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.

>        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;
>  }

<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.

> +        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)

<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"

> +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

<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.

>  
>  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.

> +
> +-- 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